Skip to content
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

Table - Sort by selected #2387

Merged
merged 61 commits into from
Sep 18, 2024
Merged

Table - Sort by selected #2387

merged 61 commits into from
Sep 18, 2024

Conversation

zamoore
Copy link
Contributor

@zamoore zamoore commented Sep 4, 2024

📌 Summary

If merged, this PR implements new "sort-by-selected" behavior in the Hds::Table component.

⚙️ Details

There is a new argument that need to be provided to the component to enable this behavior:

  • @sortBySelectedItemKey: string
    • Flag to enable SBS behavior

    • Sets which @model property is used to store the record's selection state

Screenshot 2024-09-06 at 1 30 51 PM

Jira ticket: https://hashicorp.atlassian.net/browse/HDS-3592

Copy link

vercel bot commented Sep 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Sep 17, 2024 7:24pm
hds-website ✅ Ready (Inspect) Visit Preview Sep 17, 2024 7:24pm

@zamoore zamoore force-pushed the 3779-table/sort-by-selected/code branch from 7c91a50 to 516bd22 Compare September 5, 2024 19:55
@hashibot-hds hashibot-hds added the docs-website Content updates to the documentation website label Sep 6, 2024
@zamoore zamoore force-pushed the 3779-table/sort-by-selected/code branch from 9e767c6 to 798eeb4 Compare September 6, 2024 19:52
@zamoore zamoore force-pushed the 3779-table/sort-by-selected/code branch from b2bb815 to 0af97b5 Compare September 16, 2024 14:43
Copy link
Contributor

@majedelass majedelass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a purely design side, this is already following the design patterns established previously (8px gap between buttons and text), using the right convention with the arrow up/down for letting the user know it's sortable, colored when selected, etc.

👍 Nicely done

Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delayed review – it took me a while, but it eventually clicked 😄 .
Couldn't spot anything off in my testing, so I think it's a go. I left a few non-blocking comments

packages/components/src/components/hds/table/index.hbs Outdated Show resolved Hide resolved
packages/components/src/components/hds/table/index.ts Outdated Show resolved Hide resolved
showcase/app/controllers/components/table.js Show resolved Hide resolved
Comment on lines +8 to +15
`Hds::Table::Tr`
- Added `@selectableColumnKey` argument which enables sorting by row selection state and specifies the corresponding selection state key.
- Added `@sortBySelectedOrder` argument which determines the state of the sort button in the selected item column.
- Added `@onClickSortBySelected` argument which is the callback for the sort button in the selected item column.

`Hds::Table::ThSelectable`
- Added `@onClickSortBySelected` argument which is the callback for the sort button in the selected item column.
- Added `@sortBySelectedOrder` argument which determines the state of the sort button in the selected item column.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: some of these are for internal use; I don't think we plan on documenting them all (but I might be wrong) so I suggest we add to the changelog only the arguments we will expose in the component API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was what I had originally, but @didoo suggested that I document all of the additions to subcomponents as well. Curious to hear his thoughts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of changelog, we all agree the changes should be mentioned.
For the website, personally I would document them anyway (and in case add a note clarifying that it's meant for internal use). That said, I totally get that the Table API documentation is already heavy so if you decide differently I would not oppose (but consider that, at times, we use the component API documentation too, to understand what a certain argument does).

Co-authored-by: Alex <alex-ju@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants