-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[Autocomplete] fix long option label to wrap instead of scrollbar #4416
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
1fa7ec8 to
bf575fe
Compare
size-limit report
|
db5185c to
967a5d6
Compare
61e4de4 to
871440b
Compare
Juanita-Dash
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!
8559933 to
d111db6
Compare
d111db6 to
8559933
Compare
src/types.ts
Outdated
| /** Defines a role for the action */ | ||
| role?: string; | ||
| /** Specifies that if the label is too long it will wrap instead of being hidden */ | ||
| wrapOverflow?: boolean; |
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.
This type is used by other components - can you double check the possible impact of this change? I'm guessing it'll show up as an available prop for ActionList items or other things but it wouldn't have any effect on those components.
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.
Since no other component except MappedAction actually use the prop, their will be no side effects.
Now this could be an irritation for dev since the intellisense will offer the prop even for components that don't use it. A solution for this would be to change the comment to inform devs that this prop is currently only supported by Autocomplete. This will also allow polaris maintainers to decide if and when they want to support this prop on other components.
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 wonder if it would make more sense to extend the type that's used for mapped actions directly? So instead of adding the field to the shared ActionListItemDescriptor, we would only have it be available for autocomplete.
Something along these lines (with a better name? 😂 )
interface ActionDescriptor = ActionListItemDescriptor & {
/** Specifies that if the label is too long it will wrap instead of being hidden */
wrapOverflow?: boolean;
}
...
actionBefore?: ActionDescriptor;
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.
Currently the MappedAction component has an internal interface named MappedAction which extends the ActionListItemDescriptor. Maybe we could simply move this type hire in the component hierarchy in order to be able to consume it inside AutocompleteProps 🤔
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.
Yeah, I think that makes sense. Shouldn't be called MappedAction since that's an implementation detail that we don't want to leak externally but we can remove the type extension in MappedAction and move it up to Autocomplete. Could follow the same pattern that's being used for OptionDescriptor? I defer to you on that though :)
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.
Following the proposal, I added type extension directly in the AutocompleteProps and inside the private MappedAction. This way, we do not expose our internals and we only have the new prop available for the auto complete component.
c46ffed to
ac65b0b
Compare
ac65b0b to
c982bd1
Compare
WHY are these changes introduced?
Part of #22937
Port of #4411 to v7
The problem resides mostly on small display with very long labels in the options. An easy example of this is when a merchant is trying to add tags to orders or draft orders. In that case, the merchant can select up to 40 characters, which on mobile display is too big to fit on the screen and will then generate a horizontal scrollbar.
NOTE This issue only happens when theirs no spaces in the word.
WHAT is this pull request doing?
MappedOption.Autocompletecomponent which allows the dev to specify if theMappedActionlabel should wrap it's content or not. This new prop was added due to the fact that we have another prop which controls if ellipsis are added or not at the end of the label.Before


After
How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes