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

Show index table link #2838

Open
wants to merge 2 commits into
base: rc
Choose a base branch
from
Open

Show index table link #2838

wants to merge 2 commits into from

Conversation

vvalentin-lucca
Copy link
Contributor

@vvalentin-lucca vvalentin-lucca commented Jun 4, 2024

Description

Users have expressed the need to be able to open links in tables to new tabs (via the right-click menu, for example).
To do this, these links must not be hidden and must be visible in the interface.

.indexTable-body-row-cell-action is therefore deprecated and replaced by .indexTable-body-row-cell-link, which is its unmasked equivalent, and following @BertrandPodevin's idea, we extend this link so that it can be operated on the whole line, without requiring an overflow: hidden, z-index, or superimposing itself on other interactive elements.



Capture d’écran 2024-06-04 à 12 04 23

@vvalentin-lucca vvalentin-lucca added the 🔖✨ Feature New feature (even a very small one) label Jun 4, 2024
@vvalentin-lucca vvalentin-lucca requested a review from a team as a code owner June 4, 2024 10:04
@c-3po c-3po bot added the 📖 Documentation changes Requires a Prisme update label Jun 4, 2024
@LuccaIntegration
Copy link

@CCNET-iLucca
Copy link

Tests d'interfaces

@BertrandPodevin
Copy link
Contributor

BertrandPodevin commented Jun 4, 2024

I find this change somewhat weird because

  • the right click will only work if the user interact exatly on the column containing the link but, without any visual indicator he has no way to know wich one it is or even that he can do it.
  • for screen reader, the link content won't make sense. ex <a>Daniel Hernandez</a> or <a>CP 2023 / 2024</a>

I think that for it to realy works, we should embed the content of every column into a link and find a way to provide some context to screen reader, like "show details", while also preventing redundancy and tab hell.

@vvalentin-lucca
Copy link
Contributor Author

@BertrandPodevin I agree with you.

We think it's a better solution than the completely hidden version of the link, but it's still not perfect. We thought of putting several links in each column, with a tabindex="-1" and a role="presentation", but it makes the code very heavy.

@BertrandPodevin
Copy link
Contributor

BertrandPodevin commented Jun 4, 2024

Maybe a role="link" directly on the tr + some javascript magic could do the trick ?

https://developer.mozilla.org/fr/docs/Web/Accessibility/ARIA/Roles/link_role

@vvalentin-lucca
Copy link
Contributor Author

No, it overwrites the row role of the tr and the table row disappears from the restitution.

@fbasmaison-lucca
Copy link
Contributor

Even though lines are supposed to be clickable since we are on Index Tables, some are not yet implemented with a link and moving to a cursor: auto can actually reduce the feeling of being deceived by the interface, cursor: pointer being forced at the moment.

Would changing the pointer to only the parts that are really clickable help for discovery?

Clicking anywhere on the row could still open as expected by us, but the UI feedback would actually reflect the semantic of the elements being hovered.

@LuccaIntegration
Copy link

@CCNET-iLucca
Copy link

Tests d'interfaces

@LuccaIntegration
Copy link

@CCNET-iLucca
Copy link

Tests d'interfaces

@jeremie-lucca jeremie-lucca added this to the 18.1 milestone Jun 10, 2024
@jeremie-lucca jeremie-lucca mentioned this pull request Jun 10, 2024
@jeremie-lucca jeremie-lucca modified the milestones: 18.1, 18.2 Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📖 Documentation changes Requires a Prisme update 🔖✨ Feature New feature (even a very small one)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants