Cleanup and refactors part 3 #464
Conversation
Co-authored-by: Michael Altamirano <michaeljaltamirano@gmail.com>
There was a problem hiding this comment.
37 of 40 files look good to go. The Request changes is for DesktopDropdown
Our TypeScript types and prop-types have been inconsistent in this component (and sub-components), which is understandable. Our TypeScript types only allowed string, not number, and it appears that is because the DesktopDropdown component only works when values are provided as strings. You can test this out yourself with the Storybook locally by changing the values in the options array to numbers instead of strings.
The HTML elements we use do accept other types (value?: string | number | readonly string[] | undefined), which is why our MobileDropdown works, since it relies on those native elements. Our DesktopDropdown component, however, does not, and as such any number we pass is in casted to a string in our event handlers. Because of this, when we update the state in the component, we don't see the result we're expecting.
So the changes requested here would be to make the DesktopDropdown compatible with value: number types. Feel free to @ me if you have any questions about this, and re-tag me when it's ready for re-review!
…i into components-refactor3
|
@michaeljaltamirano thanks for the feedback, I didnt wanted to dig much into Dropdown, it was sort of a confusing and just wanted to cleanup without impact. But screw that I overhauled the component, I took the requirement to allow I suppose there will be some Chromatic changes because of this All changes are contained in this last Commit 2f9c0cd |
| @@ -93,22 +94,22 @@ export const DesktopDropdown = ({ | |||
| isOpen={isOpen} | |||
| optionsContainerMaxHeight={optionsContainerMaxHeight} | |||
| role="menu" | |||
| aria-activedescendant={value} | |||
| aria-activedescendant={`${value}`} | |||
There was a problem hiding this comment.
I don't think this is quite what we want--if value is undefined or null we will then pass in "undefined" or "null".
value ? `${value}` : undefinedperhaps? (It looks like the DOM type is string | undefined).
There was a problem hiding this comment.
@michaeljaltamirano thanks for the feedback, I didnt wanted to dig much into Dropdown, it was sort of a confusing and just wanted to cleanup without impact. But screw that I overhauled the component, I took the requirement to allow
string | number | undefinedfor values (as initial value and optionType). And I reflected that in the story changes so we can test all scenarios.I suppose there will be some Chromatic changes because of this
All changes are contained in this last Commit 2f9c0cd
Yep, the chromatic tests are breaking, which I don't think should be happen for this kind of re-factor.
- It looks like we can avoid it for the
Mobilestory by using the same label: let's keep it pre-populating withFirst Optioninstead ofNumber Value. If you want to add an explicit number value option, let's not make it the first one to keep the visual tests passing.
- It looks like the changes have also changed the default behavior of the DesktopDropdown, which now auto-populates:
Before:
After: undefined option has aria-checked="true" before interaction
It does look like we're passing the actual type in the Desktop version now, which is nice, but let's button this up a bit before merging it in.
|
@michaeljaltamirano changes are in this commit 5179ca7 I just left strings and numbers as values examples and restored the original labels for the story. |
| @@ -30,9 +29,9 @@ type DropdownProps = { | |||
| optionsContainerMaxHeight?: string; | |||
| textAlign?: 'left' | 'center'; | |||
| /** | |||
| * The currently selected option. Can mount as `null` | |||
| * The currently selected option. Can be initially `undefined` | |||
There was a problem hiding this comment.
Previous versions showed that anything undefined would be pre-selected, so I think that we might want to re-word this. I'm just not sure what initially undefined means, and what the use case would be of something going from initially undefined to some truthy value.
There was a problem hiding this comment.
I appreciate the comments in onDesktopSelectChange. This LGTM. I left one other comment but it is non-blocking (though I do think it would be helpful to be clarified, lest there be confusion about it in the future re: undefined use cases.)
Since there were more refactors for Dropdowns than other components let's just be sure to especially double-check those when updating consumer apps.
Left comments in code. This could be the final cleanup. Anything u want throw in here let me know