-
-
Notifications
You must be signed in to change notification settings - Fork 408
Hide column type icon when column width is very narrow #4940
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
base: develop
Are you sure you want to change the base?
Hide column type icon when column width is very narrow #4940
Conversation
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 giving this a very quick review just to point out some issues I spotted immediately upon glancing at the diff. I'd like you to address these before I continue reviewing
- Fix lint errors
- Run Prettier
(See the Developer Guide for instructions on running the linter and code formatter.)
Additionally, I'll say: I was expecting this to be a much smaller change. I was not expecting a resize observer. I'm not sure that's actually necessary here. I think the icon is okay if the column name is truncated a bit. What I had in mind is a static column width, below which we would hide the icon. So I wouldn't imagine any changes being necessary inside Truncate. Can try simplifying this to behave like that?
| border-radius: 0.2rem; | ||
| max-width: var(--tooltip-max-width, 22rem); | ||
| } | ||
| .dropdown.truncation-content > .name-with-icon > .icon { |
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 CSS couples this component to NameWithIcon and Icon. While this sort of tight coupling is possible, we generally only use it in much higher-level components, and only as a last resort for the sake of time when developing a complex feature. For this PR, each component should not have any CSS related to other components.
| export let collapsibleIcon = false; | ||
| let isOverflowing = false; | ||
| let nwiEl: HTMLElement | undefined; |
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.
The name nwiEl has very low readability. In general please use names with full words.
|
Ah, thanks for the feedback. I'm new to all this so it's very helpful. I have a few doubts if you don't mind. How would I get the width of the header cell and update state without a resize observer? Also how much should the static width be? 50px sound good? |
f5b61f4 to
953ec2e
Compare
Fixes #4919
Hides the column type icon in the header cell when an overflow is detected
Screenshots
Checklist
Update index.md).developbranch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin