-
Notifications
You must be signed in to change notification settings - Fork 15
fix(deps): update dependency downshift to v8 #565
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
Conversation
6853a98 to
55235fb
Compare
26abee0 to
7f54c4a
Compare
7f54c4a to
faa7151
Compare
Edited/Blocked NotificationRenovate will not automatically rebase this PR, because it does not recognize the last commit author and assumes somebody else may have edited the PR. You can manually request rebase by checking the rebase/retry box above. ⚠ Warning: custom changes will be lost. |
|
@lucijanblagonic the Downshift v8 update changes listbox expansion behavior for clicking the combobox input. Our adaptation wires this up for
After thinking deeply about the pros/cons of this change, I think this upgrade is the right course of action. But I want to make sure you're onboard before moving forward. |
| <input {...getInputProps()} /> | ||
| {children} | ||
| <ul {...getListboxProps({ 'aria-label': 'Options' })} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Downshift was throwing console errors for not calling the combobox getters
geotrev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the explanations as well.
| event.preventDefault(); | ||
| } else if (isAutocomplete) { | ||
| triggerProps.onClick(event); | ||
| triggerProps.onClick && triggerProps.onClick(event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚲 triggerProps?.onClick(event) also works, but I don't feel strongly. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it does. Instead of testing for invalid triggerProps, the updated Downshift is not guaranteeing that the trigger props will contain a valid onClick ... and we can't triggerProps.onClick?(event) 😉
| ({ role = 'listbox', 'aria-labelledby': ariaLabeledBy = null, ...other }) => | ||
| getDownshiftListboxProps({ | ||
| ({ role = 'listbox', ...other }) => | ||
| getDownshiftListboxProps<any>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking re: any so I'm on the same page - is the intent here is to defer all typing to Garden's getListboxProps and prevent transient TS errors that may confuse consumers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort of. imo, while well-intended, the associated Downshift PR kind of made a mess out of the getter prop types to the point where there was no reasonable combo that could satisfy what is going on – so I hit the any eject button. At least we're coercing the options object below to be of a valid Downshift type for the function call. But this is all deeply internal to the hook code and has no implications for the well-typed external API.
This PR contains the following updates:
^7.6.0->^8.0.0Release Notes
downshift-js/downshift (downshift)
v8.0.0Compare Source
Features
BREAKING CHANGES
PRs included:
https://github.com/downshift-js/downshift/pull/1440
https://github.com/downshift-js/downshift/pull/1445
https://github.com/downshift-js/downshift/pull/1463
https://github.com/downshift-js/downshift/pull/1510
https://github.com/downshift-js/downshift/pull/1482
Changes
These changes will also be covered in the V8 migration guide.
isItemDisabled
Both
useComboboxanduseSelectnow support theisItemDisabledfunction. This new API is used to mark menu items as disabled, and as such remove the from the navigation and prevent them from being selected. The old API required passing thedisabledprop to thegetItemPropsfunction. This old API has been removed and you will receive a console warning if you are trying to use thedisabledprop in getItemProps.Example of API migration:
Old:
New:
The API for Downshift remains unchange.
useCombobox input click
ARIA 1.2 recommends to toggle the menu open state at input click. Previously, in v7, the menu was opened on receiving focus, from both mouse and keyboard. Starting with v8, input focus will not trigger any state change anymore. Only the input click event will be handled and will trigger a menu toggle. Consequently:
useCombobox.stateChangeTypes.InputFocushas been removed.useCombobox.stateChangeTypes.InputClickhas been added instead.We recommend having the default toggle on input click behaviour as it's part of the official ARIA combobox 1.2 spec, but if you wish to override it and not toggle the menu on click, use the stateReducer:
If you want to return to the v7 behaviour and open the menu on focus, keep the reducer above so you remove the toggle behaviour, and call the openMenu imperative function, returned by useCombobox, in a onFocus handler passed to
getInputProps:
Consider to use the default 1.2 ARIA behaviour provided by default in order to align your widget to the accessibility official spec. This behaviour consistency improves the user experience, since all comboboxes should behave the same and
there won't be need for any additional guess work done by your users.
Getter props return value types
Previously, the return value from the getter props returned by both Downshift and the hooks was
any. In v8 we improved the types in order to reflect what is actually returned: the default values return by the getter prop function andwhatever else the user passes as arguments. The type changes are done in this PR, make sure you adapt your TS code, if applicable.
Also, in the
Downshiftcomponent, the return values for some getter prop values have changed fromnulltoundefined, since that is what HTML elements expect (value or undefined). These values are also reflected in the TS types.nullundefined,nullundefinedhighlightedIndex >= 0 ? this.getItemId(highlightedIndex) :
nullundefinednullundefined : this.labelId,environment propTypes
The
environmentprop is useful when you are using downshift in a contextdifferent than regular DOM. Its TS type has been updated to contain
Nodeandthe propTypes have also been updated to reflect the properties which are
required by downshift from
environment.Configuration
📅 Schedule: Branch creation - "on Monday every 9 weeks of the year starting on the 3rd week" (UTC), Automerge - At any time (no schedule defined).
🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.
♻ Rebasing: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.
🔕 Ignore: Close this PR and you won't be reminded about this update again.
This PR has been generated by Mend Renovate. View repository job log here.