-
Notifications
You must be signed in to change notification settings - Fork 51
AdvancedTable - First column pinning #3104
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
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Overall, the interaction looks great. I think it would be good to add an example where you can unpin the column and it starts as pinned, currently all the examples have it default unpinned.
Also, could you add this feature to the full page demo?
packages/components/src/components/hds/advanced-table/th-context-menu.ts
Outdated
Show resolved
Hide resolved
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.
Seems to work pretty well so far
packages/components/src/components/hds/advanced-table/index.hbs
Outdated
Show resolved
Hide resolved
packages/components/src/components/hds/advanced-table/index.hbs
Outdated
Show resolved
Hide resolved
packages/components/src/components/hds/advanced-table/th-context-menu.ts
Show resolved
Hide resolved
onPinFirstColumn(); | ||
} | ||
|
||
dropdownCloseCallback?.(); |
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.
Nit: I think I like the style of typeof === 'function'
for nil checking functions, but I think we should stick to one style of checking inside of one function
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.
Also, I know this is my own code, but I think dropdownCloseCallback should always be present, right?
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.
Looking at other components typeof functionName === 'function'
seems to be the style used for nil checking functions. Was there another way of checking you wanted to use?
dropdownCloseCallback
has to be marked as optional, because in the Dropdown blocks close
is marked as optional. When setting it to required in the context menu the following errors get thrown.
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 be fixed by #3118
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.
@shleewhite As part of your PR should we update the dropdownCloseCallback
to be required?
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, the PR already makes it so D.close
which is used here is always defined
design-system/packages/components/src/components/hds/advanced-table/th-context-menu.hbs
Line 17 in cf13cb9
{{on "click" (fn option.action @column D.close)}} |
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, the PR already makes it so D.close
is always defined
design-system/packages/components/src/components/hds/advanced-table/th-context-menu.hbs
Line 17 in cf13cb9
{{on "click" (fn option.action @column D.close)}} |
packages/components/src/components/hds/advanced-table/th-sort.ts
Outdated
Show resolved
Hide resolved
5f01434
to
be88bbb
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.
Pull Request Overview
This PR adds support for first column pinning in the AdvancedTable component, allowing users to toggle the pinned state of the first column through a context menu. The changes enhance the existing @hasStickyFirstColumn
argument to support user interaction while also adding internationalization support for context menu strings.
Key changes:
- Enhanced
@hasStickyFirstColumn
to support user-controlled pinning/unpinning via context menu - Added internationalization support for context menu options
- Extended context menu functionality to include pin/unpin options for the first column
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
packages/components/src/components/hds/advanced-table/index.ts |
Core component logic for managing first column pinning state and user interactions |
packages/components/src/components/hds/advanced-table/th-context-menu.ts |
Context menu component with new pin/unpin options and i18n support |
packages/components/src/components/hds/advanced-table/th.ts |
Updated header cell component to support context menu for sticky columns |
packages/components/src/components/hds/advanced-table/th-sort.ts |
Updated sortable header cell component with context menu support |
packages/components/translations/hds/components/advanced-table/th-context-menu/en-us.yaml |
Translation file for context menu strings |
showcase/app/templates/page-components/advanced-table.hbs |
Showcase page demonstrating the new pinnable column functionality |
showcase/tests/integration/components/hds/advanced-table/index-test.js |
Comprehensive test coverage for the new pinning functionality |
.changeset/tangy-bushes-matter.md |
Changelog entry documenting the new features |
packages/components/src/components/hds/advanced-table/th-context-menu.ts
Outdated
Show resolved
Hide resolved
093b7b7
to
cf13cb9
Compare
📌 Summary
If merged, this PR would support allowing the user to pin and unpin the first column of the
AdvancedTable
. It also has a few other alterations and fixes for the Advanced table for translating text in the context menu, and fixing a CSS issues in the scroll indicator.Preview page
🛠️ Detailed description
Column pinning
If the existing
@hasStickyFirstColumn
argument istrue
currently the first column is sticky/pinned, but the user can does not have the ability to change that, or to pin a column that started as unpinned.With the changes in this PR,
@hasStickyFirstColumn
would have the following interactions:true
, the first column is sticky and the user can toggle its pinned state with the context menufalse
, the first column is not sticky and the user can toggle its pinned state with the context menuundefined
, the first column is not sticky and there is no menu option to pin itAdditionally as part of the filtering implementation all text in the
ThContextMenu
has been converted to use translations.Design considerations
With the changes proposed here, a user would be able to change a first column set as sticky to not sticky with the context menu. It would not be possible to have a sticky column that the user can not change the state of.
Scroll indicator CSS issue
While implementing the column pinning feature, it was noticed that when the column was sticky there was a slight gap present between the left scroll indicator, and the border of the sticky cells.
This is an existing issue caused when the width of the first column is set to a fixed px width. When the first column width is set to
auto
this issue is not seen.The cause of this issue is that when the first column is set to
auto
its width grows slightly when sticky as the border widths of it are increased. However, the offset calculated for the scroll indicator uses the smaller pre-scrolled width of the cell in its calculations. This allows the offset of the scroll indicator to be slightly less than the cell width and be hidden appropriately.Not sticky


Sticky
When the width of a column is set to a fixed px value, the width of the cell does not increase when sticky. When this occurs the scroll indicator is not positioned correctly since it is not hidden slightly under the cell.
To fix this issue, an update has been made to reduce the left offset of the scroll indicator by 1px when the first column has a px width.
📸 Screenshots
🔗 External links
Jira ticket: HDS-5237
👀 Component checklist
💬 Please consider using conventional comments when reviewing this PR.
📋 PCI review checklist
Examples of changes to controls include access controls, encryption, logging, etc.
Examples include changes to operating systems, ports, protocols, services, cryptography-related components, PII processing code, etc.