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

Update to latest frontend-shared with SelectNext UI enhancements #6449

Closed
wants to merge 1 commit into from

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Jul 16, 2024

Update the frontend-shared library to bring the latest SelectNext UI improvements:

EDIT: Due to the issue mentioned in this comment, this PR should not be merged until hypothesis/frontend-shared#1619 is merged.

@acelaya acelaya requested a review from robertknight July 16, 2024 10:37
@acelaya
Copy link
Contributor Author

acelaya commented Jul 16, 2024

Hmm, I think something broke, as the type of onChange's argument is no longer inferred. I'll look into it.

image

@robertknight
Copy link
Member

robertknight commented Jul 16, 2024

In hypothesis/client@c4d7404 I changed the argument type to be explicitly specified.

In my initial testing it seemed that the new types were complex enough to prevent inference from working, though I didn't distill the issue down to a minimal reproduction. If we can make inference work again that would be great. Another option might be to use create separate aliases for the component for single and multi-selection. Something along the lines of:

const Select<T> = SelectNext as ...;
const SelectMulti<T> = SelectNext as ...;

@acelaya
Copy link
Contributor Author

acelaya commented Jul 16, 2024

In hypothesis/client@c4d7404 I changed the argument type to be explicitly specified.

In my initial testing it seemed that the new types were complex enough to prevent inference from working, though I didn't distill the issue down to a minimal reproduction. If we can make inference work again that would be great. Another option might be to use create separate aliases for the component for single and multi-selection. Something along the lines of:

const Select<T> = SelectNext as ...;
const SelectMulti<T> = SelectNext as ...;

I would swear, in my initial testing in frontend-shared's sandbox, type inference kept working properly after adding multiple support, but I may have missed something. I'll take a look.

EDIT: Ok, I have realized there's no example in frontend-shared where onChange is defined inline, so that's why I didn't notice this.

EDIT 2:

Another option might be to use create separate aliases for the component for single and multi-selection. Something along the lines of:

const Select<T> = SelectNext as ...;
const SelectMulti<T> = SelectNext as ...;

In light of the problem that was introduced there, this might not be a bad solution, as it would also be a way to slowly transition away from the SelectNext symbol. We would be exporting all SelectNext , Select and SelectMulti (or perhaps MultiSelect?) and in a future major version, we would just stop exporting SelectNext and potentially rename it.

@acelaya
Copy link
Contributor Author

acelaya commented Jul 17, 2024

Superseded by #6450

@acelaya acelaya closed this Jul 17, 2024
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.

2 participants