Validate schemes for CopyToClipboardComponent#22141
Validate schemes for CopyToClipboardComponent#22141NobodysNightmare wants to merge 1 commit intodevfrom
Conversation
NobodysNightmare
force-pushed
the
copy-component-schemes
branch
from
fa6ff2d to
f6da3a3
Compare
HDinger
left a comment
HDinger
left a comment
There was a problem hiding this comment.
I am really not a fan of this component. It is a prime example for why we should not easily add "helper" components without thinking them through properly. Everything that's part of the notebook should be well defined...
Anyway (now that I'm done with ranting 😅), thanks for taking the time to update this. Looks good to me. 👍
NobodysNightmare
force-pushed
the
copy-component-schemes
branch
from
f6da3a3 to
7e21167
Compare
| super(value) | ||
|
|
||
| @scheme = scheme | ||
| @scheme = validate_scheme!(scheme) |
There was a problem hiding this comment.
You might want to consider using fetch_or_fallback (from Primer::FetchOrFallbackHelper) instead.
There was a problem hiding this comment.
This method somehow gives me left-pad vibes...
I mean, it's very simple to implement and I find it more likely that someone needs to look up what fetch_or_fallback does exactly, than that someone needs to understand what validate_scheme! is doing.
As a non-frequent primer user I might be biased towards not knowing well-established primer helpers here.
myabc
left a comment
myabc
left a comment
There was a problem hiding this comment.
Primer's fetch_or_fallback would be a more idiomatic way of validating options.
| class CopyToClipboardComponent < ApplicationComponent | ||
| include OpPrimer::ComponentHelpers | ||
|
|
||
| ALLOWED_SCHEMES = %i[value link].freeze |
There was a problem hiding this comment.
nitpick:
| ALLOWED_SCHEMES = %i[value link].freeze | |
| SCHEME_OPTIONS = %i[value link].freeze |
better aligns with Primer's naming.
When trying to use this component, I wanted to use the input scheme that I copied from elsewhere in the code. At first I was surprised that the lookbook didn't yet contain a playground, where I could try this style out. After adding it, I was surprised that it didn't look like an input at all and learned that this scheme does not even exist... But it was already used multiple times :O
NobodysNightmare
force-pushed
the
copy-component-schemes
branch
from
7e21167 to
9e21669
Compare
3 participants
When trying to use this component, I wanted to use the input scheme that I copied from elsewhere in the code. At first I was surprised that the lookbook didn't yet contain a playground, where I could try this style out.
After adding it, I was surprised that it didn't look like an input at all and learned that this scheme does not even exist... But it was already used multiple times :O
Ticket
none