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 component: adding click event on expandable table rows #2127

Merged
merged 13 commits into from
Aug 7, 2023

Conversation

ghost
Copy link

@ghost ghost commented Aug 2, 2023

Description

Table component: adding click event on expandable table rows

Ref Slack tråd https://nav-it.slack.com/archives/C7NE7A8UF/p1685454962205769)

Change summary

  • added click event on table row
  • fixed styling on table row when shadeOnHover prop is true and expandable is disabled (hover seemed confusing as the table has no interaction)

@changeset-bot
Copy link

changeset-bot bot commented Aug 2, 2023

🦋 Changeset detected

Latest commit: 14465d8

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

This PR includes changesets to release 7 packages
Name Type
@navikt/ds-css Minor
@navikt/ds-react Minor
@navikt/aksel-stylelint Minor
@navikt/aksel Minor
@navikt/ds-tokens Minor
@navikt/ds-tailwind Minor
@navikt/aksel-icons 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

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

Endringer til review: 1

6a6f64fe2 | 47 komponenter | 299 stories

marionhauffnav and others added 2 commits August 2, 2023 14:05
Fjerner unntaket for når raden er åpen, slik at pointer-cursor vises også når brukeren skal lukke raden.

Co-authored-by: Vegard Haugstvedt <it.vegard@gmail.com>
</Table>
);
},
play: async ({ canvasElement, step }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Laget en interaktiv demo (kan ses i storybook) for et av problemene som har stoppet oss fra denne oppdateringen tidligere:

I tilfeller der man har andre interaktive elementer som eks lenker, checkboxer eller knapper ønsker vi at man ikke skal åpne raden ved klikk på dem. Dette kan kanskje også videreføres til at raden ikke skal åpnes hvis man ønsker å eks kopiere noe tekst, men ikke sikker på den.

Arbeidet stoppet litt opp med å finne en god løsning på dette, men kan hende alt man trenger er å sjekke document.activeElement 🤞 Vært helt mega å finne en god løsning på dette da jeg vet mange ønsker seg denne featuren 🙌

Copy link
Author

Choose a reason for hiding this comment

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

Takk for eksemplet.
Eventet vil per default spre seg videre ja. Dersom de interaktive elementene skal hindre eventet til å gå videre til raden må det ansvaret være på dem. Jeg la til en event.stopPropagation i click-handlere på både Button og Checkbox i eksempelet.
Jeg kommenterte også ut "play" bitten slik at alerts ikke kjører med en gang.

Copy link
Author

@ghost ghost Aug 3, 2023

Choose a reason for hiding this comment

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

🐡 Update: Har lagt til en ny commit igjen nå, hvor jeg fjerner stopPropagation fra elementene i storyen, og istedet legger til logikk på selve ExpandableRow for å kun ekspande ved behov (Dvs når den registrerer klikk på selve raden, eller ekspandable-knappen, men ikke ellers). Se linje 73-89.

🍄 Det kan være en mer robust løsning som gjør at konsumentene ikke behøver å forholde seg til propagation i det hele tatt.


const onRowClick = (e) => {
if (
e.target.nodeName === "TD" ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Selv om vi ikke har noen kodeeksempler på det (og stylingen for det virker litt "off"), så skulle jeg gjerne ønsket at vi sjekker for "TH" i tillegg til "TD", i tilfelle noen bruker radoverskrifter. Mest for å forhindret at vi glemmer dette om man tar i bruk radoverskrifter i fremtiden.

marionhauffnav and others added 3 commits August 7, 2023 11:08
Copy link
Collaborator

@KenAJoh KenAJoh left a comment

Choose a reason for hiding this comment

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

🚢 🇮🇹 Ser bra ut!

@KenAJoh KenAJoh merged commit e033b34 into main Aug 7, 2023
@KenAJoh KenAJoh deleted the mh/expandable-table-click-area branch August 7, 2023 11:42
@github-actions github-actions bot mentioned this pull request Aug 7, 2023
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