Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add SelectField.mapOption prop. #195

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

stephenh
Copy link
Contributor

@stephenh stephenh commented Jul 27, 2021

Trying out an alternative API for SelectField.

Having to type out getOptionLabel and getOptionValue all the time has been boilerplately.

Our current attempt to reduce this was some type system magic to realize "oh the option already has an id+name, we'll just use that", and then make getOptionLabel and getOptionValue optional for just that one specific type of option.

But it was pretty ugly to pull off, and also very limited, you had to implement id+name to get the succinct versions.

Playing around, it seems cute to combine getOptionLabel + getOptionValue + getOptionMenuLabel into a single prop of "hey we need you to map this option to 'stuff we need'".

The upshot of it being a single prop is that, even when defining by hand it's more succinct, but also you can apply defaults for all three behaviors from a single function, i.e. idAndName:

        <TestSelectField
          label="Favorite Icon - Disabled"
          value={undefined}
          options={options}
          disabled
          mapOption={idAndName}
        />

The one downside is that, if you did have id+name now you have the extra boilerplate of passing mapOption={idAndName} ... however, I think the majority of our callers didn't magically have id+name anyway, and if anything this will be easier to make more idAndName-ish functions for common combinations i.e. idAndLabel or what not.

Opening this up to kinda think about. So far I'm bullish that this is cleaner/better, but not going to rush merging it.

@stephenh stephenh marked this pull request as draft August 3, 2021 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant