Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Adding features to tables #859

Merged
merged 26 commits into from
Dec 21, 2022
Merged

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Oct 19, 2021

Need a backport to v4 ?

Added features :

Fixed :

@louismaximepiton
Copy link
Member Author

Added specs that are not in Zeplin at the moment.
Artboard2
Artboard

@louismaximepiton louismaximepiton marked this pull request as ready for review October 26, 2021 09:31
@julien-deramond julien-deramond changed the title Draft : Adding features to tables Adding features to tables Oct 28, 2021
@louismaximepiton louismaximepiton force-pushed the main-louismaximepiton-tables branch from 6d517b1 to f836fc3 Compare January 21, 2022 09:30
@julien-deramond

This comment was marked as resolved.

@louismaximepiton louismaximepiton force-pushed the main-louismaximepiton-tables branch from f836fc3 to d8eb752 Compare March 2, 2022 10:56
@louismaximepiton louismaximepiton force-pushed the main-louismaximepiton-tables branch from d8eb752 to 43b50ca Compare April 4, 2022 10:02
@louismaximepiton louismaximepiton force-pushed the main-louismaximepiton-tables branch from 43b50ca to c6fdf54 Compare April 27, 2022 12:28
@julien-deramond julien-deramond removed their request for review May 3, 2022 05:36
@isabellechanclou isabellechanclou force-pushed the main-louismaximepiton-tables branch from c6fdf54 to 26884ba Compare May 4, 2022 07:25
@isabellechanclou isabellechanclou self-requested a review May 4, 2022 08:25
@Aniort
Copy link
Contributor

Aniort commented May 12, 2022

hello,
some a11y comments:

  • no need to add tabindex="0" on div (content (even table) with scrollbar are keyboard usable
  • we must warn boosted users that they don't always need role="region" and" aria-label="xxx" if there is only one table as main content of the page, or if tables are just considered as a normal, common content. But if they are important when using the application, it could be important f"or some users to identify them specifically with a role=region and an aria-label to be easily acceded
  • and colspan are not an a11y barrier for screenreaders but, if not definitely needed, shouldn't be used to avoid misunderstanding

@louismaximepiton louismaximepiton force-pushed the main-louismaximepiton-tables branch from 26884ba to ac6314b Compare June 7, 2022 15:52
@netlify
Copy link

netlify bot commented Jun 7, 2022

Deploy Preview for boosted ready!

Name Link
🔨 Latest commit b509531
🔍 Latest deploy log https://app.netlify.com/sites/boosted/deploys/63a2d5b20b77f000088059e7
😎 Deploy Preview https://deploy-preview-859--boosted.netlify.app/docs/5.2/content/tables
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

louismaximepiton and others added 5 commits August 10, 2022 12:15
Signed-off-by: louismaximepiton <louismaxime.piton@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
Signed-off-by: Isabelle Chanclou <isabelle.chanclou@orange.com>
@julien-deramond

This comment was marked as outdated.

@Franco-Riccitelli
Copy link
Member

Here is my design review for Tables.

Striped columns:

  • Striped columns do not exist in the Web UI Kit.

Row Selection:

  • The row is selected by clicking on the checkbox.
  • In the example, the row does not select or deselect when clicking the checkbox.
  • Same comment for the 'With icons or thumbnails' example.
  • The grey on the selected state (in the Row selection example) should be #DDDDDD. In the example it's #DEDEDE.

Table group dividers:

  • Table group dividers are not included in the Web UI Kit.

Nested tables:

  • I think this table should use the 'Expanding Row' de#cluded in the UI kit.
  • The example should be on a black or white background.
  • Do not nest a white table in a black table.
  • The row that is expanded should be in the selected state. Refer to UI Kit example.
    Link: https://system.design.orange.com/0c1af118d/p/21b04a-data-tables

@louismaximepiton
Copy link
Member Author

Hi @Franco-Riccitelli,

First of all, thanks a lot for the time you invested into this review !

No changes

To give you feedback on what you mentionned, we'll add some danger callouts on Striped columns, Table group dividers and Nesting. Indeed, it seems that these aren't in the DSM and as you said, there's an alternative to the one people might want to use.

Changes

On the subject of Row selection:

  • The selecting feature is not implemented yet and will be released as an example. We already have a callout on this so I didn't change anything. Same for the icons and thumbnails section.
  • I changed the gray that was a mistake of mine.

Feedback

If you're fine with the changes, could you please write a seal of approval ?

@Franco-Riccitelli
Copy link
Member

Hi @louismaximepiton

Those changes are all good. Thanks.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@julien-deramond julien-deramond merged commit 9383bd1 into main Dec 21, 2022
@julien-deramond julien-deramond deleted the main-louismaximepiton-tables branch December 21, 2022 14:14
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants