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: add tdsClick event to table rows #760

Closed
wants to merge 31 commits into from

Conversation

ckrook
Copy link
Collaborator

@ckrook ckrook commented Sep 17, 2024

Describe pull-request

This pull-request is regarding adding an tdsClick event to the table rows handle events on a row click.

Issue Linking:

Choose one of the following options

How to test

Provide detailed steps for testing, including any necessary setup.

  1. Go to storybook and navigate to the table component > basic row.
  2. Check in the console that the event is being triggered upon clicking a row.

Checklist before submission

  • All existing tests pass
  • I have updated the documentation (if applicable)

Suggested test steps

  • Browser testing (Chrome, Safari, Firefox)
  • Events

Copy link
Contributor

github-actions bot commented Sep 17, 2024

Playwright test results

passed  387 passed

Details

stats  387 tests across 130 suites
duration  54.3 seconds
commit  14b56fe

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-760.d3fazya28914g3.amplifyapp.com

Copy link
Contributor

@nathalielindqvist nathalielindqvist left a comment

Choose a reason for hiding this comment

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

Great job, it works well! 🥳 Reading the issue our user submitted they suggest to add a pointer styling to the indicator if the tdsClick event is defined. Is that something we would like to add in this PR?

mistermalm and others added 3 commits September 18, 2024 16:39
* fix(card): toggle stretch mode

* chore(card): removed unnecessary comment

* Update packages/core/src/components/card/card.stories.tsx

Co-authored-by: Tim <[email protected]>

* Update packages/core/src/components/card/card.stories.tsx

Co-authored-by: Nathalie Lindqvist <[email protected]>

---------

Co-authored-by: Jonatan Malm <[email protected]>
Co-authored-by: Tim <[email protected]>
Co-authored-by: Nathalie Lindqvist <[email protected]>
@mJarsater
Copy link
Collaborator

mJarsater commented Sep 20, 2024

Great job, it works well! 🥳 Reading the issue our user submitted they suggest to add a pointer styling to the indicator if the tdsClick event is defined. Is that something we would like to add in this PR?

I don't think that is gonna work since the tdsClick event is created inside of the component by us and dispatch whether the users adds an event listener or not.

I don't know if you wanna consider making the while row tabable though? If a user is expected to press a row to perform an action I think they should also be able to tab to it, for a11y.

Edit: Making it tabable would create the same problem as abovve though. There is not way (afaik) to know whether an element has an eventlistener attach to it or not. So if you want to make it tablable and use cursor:pointer if there is a click event added I think the only option is to create another prop. Having something like clickable that the use can add if they also attach an eventlistener.

My suspicion is that the user i coming from React or Angular where the event listener would be passed as a prop and we could then simply check if the prop !== undefined. But I don't think that's gonna work with stencil.

* fix(table): passing on unit value with horizontalscroll

* fix(table): pass unit to toolbar and footer

* build: release script and wrappers improvement (#737)

* build: release script and wrappers improvement

* chore: package-json update

* fix: ignore npmignore and add package.json

* chore: packages version update

* Revert "build: release script and wrappers improvement (#737)"

This reverts commit 6e6afbd.

---------

Co-authored-by: theJohnnyMe <[email protected]>
@timrombergjakobsson
Copy link
Contributor

Great job, it works well! 🥳 Reading the issue our user submitted they suggest to add a pointer styling to the indicator if the tdsClick event is defined. Is that something we would like to add in this PR?

I don't think that is gonna work since the tdsClick event is created inside of the component by us and dispatch whether the users adds an event listener or not.

I don't know if you wanna consider making the while row tabable though? If a user is expected to press a row to perform an action I think they should also be able to tab to it, for a11y.

Edit: Making it tabable would create the same problem as abovve though. There is not way (afaik) to know whether an element has an eventlistener attach to it or not. So if you want to make it tablable and use cursor:pointer if there is a click event added I think the only option is to create another prop. Having something like clickable that the use can add if they also attach an eventlistener.

My suspicion is that the user i coming from React or Angular where the event listener would be passed as a prop and we could then simply check if the prop !== undefined. But I don't think that's gonna work with stencil.

@mJarsater good catch! Thanks a lot for the feedback. I believe that browsers don’t expose APIs to inspect or retrieve a list of event listeners associated with a particular DOM element? So like you said, it won't work. So I guess we would need to manually ask the user to declare intent (like through a clickable prop). The prop tells the component: "Yes, I want this row to be clickable," without relying on detecting event listeners.

@ckrook
Copy link
Collaborator Author

ckrook commented Sep 24, 2024

tab-table.mov

Is this the look we want to have for the table if the clickable is true? (I'm thinking about the outline style on click / Enter keypress)

Comment on lines +68 to +70
:host(.tds-table__row--clickable:focus-visible) {
outline: var(--focus-outline, 2px solid blue);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

To answer your question about style of focus state:
Instead, you can use mixin from packages/core/src/mixins/_focus-state.scss.

@theJohnnyMe
Copy link
Contributor

theJohnnyMe commented Sep 25, 2024

I can not get tdsClick working either in preview link or in my local after pulling branch. Not sure what is off, please investigate.
I get this error in console, not sure if it is related to tdsClick not emitting on click:
Screenshot 2024-09-25 at 15 30 31

Lunkan89 and others added 19 commits October 1, 2024 15:15
* feat: added skip to first and last buttons and dropdown for rows per page

* fix(table): using scss vars for styling in nested shadowroots

* fix(dropdown): renaming story variables

* fix(table-footer): rows per page default value from passed values array
Bumps [send](https://github.com/pillarjs/send) and [express](https://github.com/expressjs/express). These dependencies needed to be updated together.

Updates `send` from 0.18.0 to 0.19.0
- [Release notes](https://github.com/pillarjs/send/releases)
- [Changelog](https://github.com/pillarjs/send/blob/master/HISTORY.md)
- [Commits](pillarjs/send@0.18.0...0.19.0)

Updates `express` from 4.18.2 to 4.21.0
- [Release notes](https://github.com/expressjs/express/releases)
- [Changelog](https://github.com/expressjs/express/blob/4.21.0/History.md)
- [Commits](expressjs/express@4.18.2...4.21.0)

---
updated-dependencies:
- dependency-name: send
  dependency-type: indirect
- dependency-name: express
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [body-parser](https://github.com/expressjs/body-parser) and [express](https://github.com/expressjs/express). These dependencies needed to be updated together.

Updates `body-parser` from 1.20.2 to 1.20.3
- [Release notes](https://github.com/expressjs/body-parser/releases)
- [Changelog](https://github.com/expressjs/body-parser/blob/master/HISTORY.md)
- [Commits](expressjs/body-parser@1.20.2...1.20.3)

Updates `express` from 4.19.2 to 4.21.0
- [Release notes](https://github.com/expressjs/express/releases)
- [Changelog](https://github.com/expressjs/express/blob/4.21.0/History.md)
- [Commits](expressjs/express@4.19.2...4.21.0)

---
updated-dependencies:
- dependency-name: body-parser
  dependency-type: indirect
- dependency-name: express
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
#771)

Bumps [rollup](https://github.com/rollup/rollup) from 4.20.0 to 4.22.4.
- [Release notes](https://github.com/rollup/rollup/releases)
- [Changelog](https://github.com/rollup/rollup/blob/master/CHANGELOG.md)
- [Commits](rollup/rollup@v4.20.0...v4.22.4)

---
updated-dependencies:
- dependency-name: rollup
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…776)

Bumps [rollup](https://github.com/rollup/rollup) from 2.79.1 to 2.79.2.
- [Release notes](https://github.com/rollup/rollup/releases)
- [Changelog](https://github.com/rollup/rollup/blob/master/CHANGELOG.md)
- [Commits](rollup/rollup@v2.79.1...v2.79.2)

---
updated-dependencies:
- dependency-name: rollup
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* feat: add the possibility to flex expanded row in table

* feat: delegate overflow layout control to users in expandable table row

* fix: adjest the storybook demo block

* fix: cleanup code

* fix: clean up test screenshot

* chore: remove background pink on table expandable row

* Update packages/core/src/components/table/table-body-row-expandable/table-body-row-expandable.tsx

Co-authored-by: theJohnnyMe <[email protected]>

* fix(table): update docs from scroll to auto

* fix(table-expand-row): use auto instead of scroll for overflow

---------

Co-authored-by: theJohnnyMe <[email protected]>
Bumps [webpack](https://github.com/webpack/webpack) to 5.94.0 and updates ancestor dependency [@angular-devkit/build-angular](https://github.com/angular/angular-cli). These dependencies need to be updated together.


Updates `webpack` from 5.90.3 to 5.94.0
- [Release notes](https://github.com/webpack/webpack/releases)
- [Commits](webpack/webpack@v5.90.3...v5.94.0)

Updates `@angular-devkit/build-angular` from 17.3.8 to 17.3.10
- [Release notes](https://github.com/angular/angular-cli/releases)
- [Changelog](https://github.com/angular/angular-cli/blob/main/CHANGELOG.md)
- [Commits](angular/angular-cli@17.3.8...17.3.10)

---
updated-dependencies:
- dependency-name: webpack
  dependency-type: indirect
- dependency-name: "@angular-devkit/build-angular"
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* feat(table): zebra rows

* fix: unused state

* fix(table): rows & columns zebra mode

* docs: readme updates

* fix: hover on columns and rows

* fix: renaming variable

* test(table): added zebra mode tests

* fix: dry in styling
* fix: added to before submission checklist

* fix: added to before submission checklist
Copy link

sonarcloud bot commented Oct 9, 2024

Copy link
Contributor

@nathalielindqvist nathalielindqvist left a comment

Choose a reason for hiding this comment

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

Has something happened in this PR? There seems to be 67 files changed, was there a rebase that went out of bounce?

Copy link
Contributor

@nathalielindqvist nathalielindqvist left a comment

Choose a reason for hiding this comment

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

The tdsClick event returns the tableIds as undefined, is this expected behavior?
Screenshot 2024-10-10 at 13 11 33

@ckrook ckrook closed this Oct 10, 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.

7 participants