Fix select input components dropdown menu width #2699
Conversation
🦋 Changeset detectedLatest commit: 54bbd00 The changes in this PR will be included in the next version bump. This PR includes changesets to release 94 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| 'minMenuWidth', | ||
| Constraints.getAcceptedMaxPropValues(2), | ||
| 3 | ||
| )} | ||
| maxMenuWidth={select( | ||
| 'maxWidth', | ||
| Constraints.getAcceptedMaxPropValues(), | ||
| 'maxMenuWidth', | ||
| Constraints.getAcceptedMaxPropValues(4), |
There was a problem hiding this comment.
Just using better names for the knobs and use the proper default and available values.
| props.maxMenuWidth ?? getHorizontalConstraintValue(props.maxMenuWidth), | ||
| maxWidth: props.maxMenuWidth | ||
| ? getHorizontalConstraintValue(props.maxMenuWidth) | ||
| : designTokens.constraintScale, |
There was a problem hiding this comment.
If no maxMenuWidth is provided, we use 'scale' as the default value.
| designTokens.constraint3 | ||
| ); | ||
| const designTokenSuffix = | ||
| typeof value === 'string' ? upperFirst(value) : value; |
There was a problem hiding this comment.
If the scale value is provided, we need a little transformation as its related token name is constraintScale.
| const constraintValue = | ||
| designTokens[`constraint${designTokenSuffix}` as TDesignTokenName] || | ||
| designTokens.constraint3; |
There was a problem hiding this comment.
I think if the current minMenuWidth or maxMenuWidth is auto, then constraintValue will fall back to designTokens.constraint3 here. Not sure if this is correct.
There was a problem hiding this comment.
Yes, I also have doubts about this. Maybe @FilPob can help us what should the 'auto' prop do in this case?
There was a problem hiding this comment.
Thanks for pointing that out.
I had another implementation alternative that I discarded at last minute but I think is better.
Please take a look at the changes here: 54bbd00
There was a problem hiding this comment.
Thanks, Carlos. I agree, this is a better solution.
There was a problem hiding this comment.
Honestly I am also not very sure what that auto option is doing :D But maybe it doesnt really make sense to have that option available for min.width and max.width.... I can image if you have just one "width" as a prop that the value gets calculated automatically somehow. But setting both min.width and max.width to auto doesnt make sense does it?
There was a problem hiding this comment.
Let's try to get this merged so we fix the bug and we can customize the available options as a follow-up 🙏
| @@ -94,13 +94,17 @@ type TState = { | |||
|
|
|||
| type TDesignTokenName = keyof typeof designTokens; | |||
|
|
|||
| const upperFirst = (str: string) => str.charAt(0).toUpperCase() + str.slice(1); | |||
There was a problem hiding this comment.
Perhaps we can use upperFirst from lodash, since it's already a dependency in this package
There was a problem hiding this comment.
I checked the main package.json file and didn't see that dependency so I assumed we were not using it in this repository 🤦
Anyway, with the new approach we don't need that utility anymore.
| default: | ||
| return ( | ||
| designTokens[`constraint${value}` as TDesignTokenName] || | ||
| designTokens.constraintScale |
There was a problem hiding this comment.
From this helper point of view, I think it's safer to default to 100%.
Using this approach, even if we don't transform the incoming scale value, we will return that value anyway.
Summary
Fix select input components dropdown menu width
Description
We have an issue while calculating the width for the dropdown menu rendered when a select input component is clicked.
The problem is that we are using the minimum width configured also for the maximum width of that element: