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

feat(table): added sorting, cell types, actions, and inputs #2444

Merged
merged 8 commits into from
Sep 26, 2024

Conversation

ArtBlue
Copy link
Contributor

@ArtBlue ArtBlue commented Sep 13, 2024

Fixes #1900

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

This PR is Iteration 2 of the Table module. It includes

  1. Sorting
  2. Cell Types
  3. Actions
  4. Inputs

Notes

The BEM structure was adjusted to better align with the markup structure.

Screenshots

image

image

image

image

Checklist

  • I verify the build is in a non-broken state
  • I verify all changes are within scope of the linked issue
  • I regenerated all CSS files under dist folder
  • I tested the UI in all supported browsers
  • I did a visual regression check of the components impacted by doing a Percy build and approved the build
  • I tested the UI in dark mode and RTL mode
  • I added/updated/removed Storybook coverage as appropriate

Copy link

changeset-bot bot commented Sep 13, 2024

🦋 Changeset detected

Latest commit: a6151bd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@ebay/skin Minor

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

Copy link
Contributor

@agliga agliga left a comment

Choose a reason for hiding this comment

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

Took a quick pass. Looks good overall, but I had a concern about the amount of css variables we added.

src/sass/table/table.scss Outdated Show resolved Hide resolved
@ianmcburnie
Copy link
Contributor

Seeing an odd artifact on the previously keyboard focussed element in Safari:

Screenshot 2024-09-17 at 1 40 57 PM

@ArtBlue
Copy link
Contributor Author

ArtBlue commented Sep 20, 2024

Seeing an odd artifact on the previously keyboard focussed element in Safari:

@ianmcburnie , this is fixed.

@agliga
Copy link
Contributor

agliga commented Sep 20, 2024

One thing I noticed is one of the examples looks misaligned.
The Multiple Actions seem to be bleeding into Actions Plus, is this expected?
I would expect that column to expand to fix the content.
Screenshot 2024-09-20 at 12 27 34 PM

@agliga
Copy link
Contributor

agliga commented Sep 20, 2024

To add (forgot to be explicit in my last comment): Im fine approving it other than that alignment thing, looks ready to go to me!

@ArtBlue
Copy link
Contributor Author

ArtBlue commented Sep 20, 2024

One thing I noticed is one of the examples looks misaligned. The Multiple Actions seem to be bleeding into Actions Plus, is this expected? I would expect that column to expand to fix the content.

Great catch!!! I fixed it.

Technically, there is a max cell width, so it's not an indeterminate amount of space, but it was nowhere near the max; was a real issue.

Copy link
Contributor

@agliga agliga left a comment

Choose a reason for hiding this comment

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

LGTM! Nice job

@ianmcburnie
Copy link
Contributor

ianmcburnie commented Sep 24, 2024

Could we please add an introductory sentence for the sections "Compact Density Table" and "Relaxed Density Table"? I think those modifiers just affect the spacing, right?

Generally I think we should always have a sentence of explanation/introduction after a heading.

@ianmcburnie
Copy link
Contributor

ianmcburnie commented Sep 24, 2024

I notice that the Basic Default Table includes sortable column headers. How do we feel about making the basic version non-sortable and introducing the concept of column sort afterwards (i.e. create a new example section specially for that).

Generally I like to layer in the complexity/features. It might make sense in this case also.

@ianmcburnie
Copy link
Contributor

ianmcburnie commented Sep 24, 2024

For the introduction, I'd be tempted to just align with the MDN text:

The <table> HTML element represents tabular data—that is, information presented in a two-dimensional table comprised of rows and columns of cells containing data.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/table

We can probably remove this text as I don't see it providing much value:

Interactive elements should be tabbed into in correct sequence top down row by row and column by column based on the respective LTR or RTL mode.

I would change this sentence:

The top-level container element should have a tabindex="0" to make it focusable, an aria-label to relay context of the component, and should have a role of group to make it accessible to screen readers. That will also allow the table element to be scrollable inside the container, both vertically (when overflowing) and horizontally.

to something like this:

To allow scrolling of the table via a keyboard interface, the root element must be added to the page's tab sequence. This in turn requires the root element to have an accessible role and label.

I've been meaning to sweep through the entire docs to align with a single style & tone of voice.

@ArtBlue
Copy link
Contributor Author

ArtBlue commented Sep 24, 2024

@ianmcburnie ,

For the introduction, I'd be tempted to just align with the MDN text

I thought about doing something similar, but the module's outermost element is not a table. I would actually prefer we keep it as is to allow more flexibility. Lining up modules directly with html elements is not ideal IMO.

I'll change the language to be more inline with what you have, but will avoid reference to the html element.

Copy link
Contributor

@ianmcburnie ianmcburnie left a comment

Choose a reason for hiding this comment

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

Very nice!

@ArtBlue ArtBlue merged commit 5e59592 into 18.3.0 Sep 26, 2024
@ArtBlue ArtBlue deleted the 1900-table-iteration2-fixed branch September 26, 2024 19:09
@github-actions github-actions bot mentioned this pull request Sep 26, 2024
@github-actions github-actions bot mentioned this pull request Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants