-
Notifications
You must be signed in to change notification settings - Fork 25
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
PANGOLIN-3680: data table settings menu #2797
PANGOLIN-3680: data table settings menu #2797
Conversation
🦋 Changeset detectedLatest commit: 2e9599d The changes in this PR will be included in the next version bump. This PR includes changesets to release 96 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 ↗︎
|
packages/components/data-table-manager/src/data-table-settings/data-table-settings.tsx
Show resolved
Hide resolved
triggerElement={ | ||
<IconButton icon={<ColumnsIcon />} label="list" /> | ||
} | ||
menuHorizontalConstraint={4} |
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.
menuHorizontalContraint
with value of 3
would wrap the text in the dropdown so I upped it to 4
as that was the closest I could get to the design and ensuring no wrapping occurred.
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 think we need to tag @FilPob here to approve of this decision.
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 think that's a good option, but bear in mind this adjustment works for english translation but might not work for other languages.
For instance, if we use portuguese, the text will be wrapped anyway.
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.
Awesome work 💯
Please remember to add a changeset 🙏🏾.
triggerElement={ | ||
<IconButton icon={<ColumnsIcon />} label="list" /> | ||
} | ||
menuHorizontalConstraint={4} |
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 think we need to tag @FilPob here to approve of this decision.
6b7fee4
to
dcad9a7
Compare
@kterry1 Could you please try to set the |
I think we should not merge this PR until this issue is fixed. |
@CarlosCortizasCT Is Shield planning on working on this, or is this something Pangolin Team needs to pull in? |
d7127f7
to
195417a
Compare
Hi @kterry1 We will work on it and we will try to do it this week (reference). |
195417a
to
16d35f7
Compare
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.
Looks good to me 👍
Thanks Kevin!
Summary
The DataTableManager's table settings has been replaced with the new DropdownMenu component and the tooltip as been added.