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): replaced column-type of "row-header" with boolean row-header #2355

Open
wants to merge 4 commits into
base: 15.0.0
Choose a base branch
from

Conversation

WinkeeFace
Copy link
Contributor

Description

Replaced column-type of row-header with a boolean row-header in the ebay-table component. Updated the relevant Marko and JSON files to reflect this change.

Context

This change was made to simplify the handling of row headers and improve ARIA accessibility. The boolean row-header is more intuitive and aligns better with accessibility standards.

References

Screenshots

N/A

@WinkeeFace WinkeeFace linked an issue Dec 24, 2024 that may be closed by this pull request
@WinkeeFace WinkeeFace changed the base branch from master to 15.0.0 December 24, 2024 14:59
@WinkeeFace WinkeeFace requested a review from a team December 31, 2024 20:44
Copy link
Member

@LuLaValva LuLaValva left a comment

Choose a reason for hiding this comment

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

Well done, thanks @WinkeeFace! Just a few notes:

  1. Those tests are failing because you didn't include rowHeader in the index.marko destructure here.
  2. Since Marko 5 / eBayUI normalizes kebab-case to camelCase so both are accepted, that needs to be accommodated for in TypeScript. To do this, you can either add duplicate "row-header"?: boolean; and "column-type"?: ... types in the interface (probably not recommended), or you can switch everything to kebab-case and use WithNormalizedProps<> like we do for Input (preferred).
  3. =true is implied in Marko attributes (same as HTML), so there are a few examples that can be simplified (I showed one example in a code comment)

src/components/ebay-table/examples/default.marko Outdated Show resolved Hide resolved
@WinkeeFace
Copy link
Contributor Author

Well done, thanks @WinkeeFace! Just a few notes:

  1. Those tests are failing because you didn't include rowHeader in the index.marko destructure here.
  2. Since Marko 5 / eBayUI normalizes kebab-case to camelCase so both are accepted, that needs to be accommodated for in TypeScript. To do this, you can either add duplicate "row-header"?: boolean; and "column-type"?: ... types in the interface (probably not recommended), or you can switch everything to kebab-case and use WithNormalizedProps<> like we do for Input (preferred).
  3. =true is implied in Marko attributes (same as HTML), so there are a few examples that can be simplified (I showed one example in a code comment)

Thank you for the feedback! I'll make those corrections.

Copy link

changeset-bot bot commented Jan 2, 2025

⚠️ No Changeset found

Latest commit: 21f9d1b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

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.

ebay-table: various fixes
2 participants