-
Notifications
You must be signed in to change notification settings - Fork 17
Upgrade AutocompleteWithPagination styling #7194
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
Upgrade AutocompleteWithPagination styling #7194
Conversation
'& .MuiTypography-root': { | ||
maxWidth: 497, | ||
whiteSpace: 'nowrap', | ||
overflow: 'hidden', | ||
textOverflow: 'ellipsis', | ||
}, |
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'm actually very hesitant to approve something like this because Autocomplete is used in so many places, and I don't fully understand the ramifications of these changes.
Particularly unsure about the hard coded values. I wonder would this affect other places Autocomplete is used?
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 would affect other places for Autocomplete. Though shouldn't be harmful bur rather helpful.
@zachariah-at-msupply can you post some screenshots of it's supposed to look? (preferably a few in different contexts)? It's not looking right for me, just wondering if you're seeing something different? |
It also crosses my mind that we might want to add a maximum length for table columns, with an option to expand it to show the full value? The table is scrollable so no information is hidden, but it might be a bit inconvenient at times. Screen.Recording.2025-04-15.at.11.15.05.mov |
Bundle size differenceComparing this PR to
|
Hi - what's holding up this PR? |
client/packages/common/src/ui/components/inputs/Autocomplete/AutocompleteList.tsx
Outdated
Show resolved
Hide resolved
I've come across these other issues (listed below) which are very similar. Shoul I include them in this PR, or create new issues for them? Seeing these other issues also makes me feel uneasy about the fix I have made - it seems like more sweeping changes are necessary than these fixes, which feel like they might be putting a plaster on a bigger problem. What are your thoughts? I'll await advice before spending time on them. Here are the 3 things I found:
|
Hey @zachariah-at-msupply, I'm still seeing this in the Cold Chain->Equipment Store Selection, which is the one raised in the original issue. |
This PR will now just fix the AutocompleteWithPagination component, subsequent PRs will introduce the upgrade the other components that need it. N.B. I reverted the previous change to the store autocomplete (commit db2bbe9 above) as I did not know it was worth introducing a fixed width to the autocomplete for an issue that no one has actually raised - I think that might require a bit of design input, and is probably linked to the wider issue of fixing styling throughout the application. |
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.
Okay, I think we should get this in now, the whole issue has become too confused. It looks good for the Store selector in Assets, and that's the main thing for this specific issue. If further changes are needed we can do them in a new issue (with much more specific requirements!)
Thanks @zachariah-at-msupply !
Fixes part of #5336
π©π»βπ» What does this PR do?
Upgrades the styling of the options for a specific component (AutocompleteWithPagination) to truncate options with ellipses rather than wrapping them. (N.B. This only partially solves the original issue, all the other autocomplete components will need to be fixed too, but that will happen in a subsequent PR.)
π Any notes for the reviewer?
I hope it looks good :)
π§ͺ Testing
π Documentation
π Reviewer Checklist
The PR Reviewer(s) should fill out this section before approving the PR
Breaking Changes
Issue Review
Tests Pass