hypothesis / lms

LTI app for integrating with learning management systems
BSD 2-Clause "Simplified" License
46 stars 14 forks source link

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

Closed acelaya closed 1 month ago

acelaya commented 1 month ago

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 https://github.com/hypothesis/frontend-shared/pull/1619 is merged.

acelaya commented 1 month ago

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

image

robertknight commented 1 month ago

In https://github.com/hypothesis/client/pull/6444/commits/c4d74044933e66aab42eb6a958df04438ebbede3 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 commented 1 month ago

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 commented 1 month ago

Superseded by https://github.com/hypothesis/lms/pull/6450