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: striped tables #489

Merged
merged 5 commits into from
Aug 3, 2021
Merged

feat: striped tables #489

merged 5 commits into from
Aug 3, 2021

Conversation

Lalaluka
Copy link
Contributor

Hi πŸ§‘β€πŸ’»,
Here is the Implementation of #483
I added the prop striped to the tables adapting the naming of Bootstrap.
It colors every second Row with grey 0. And can be changed through the css-variable:
--background-secondary-tbody
I'm not that happy with the Variable name (just changed it again) would --background-striped better?

Also added to Visual Tests, Storybook and Sketch Library :)

Another thing the Screenshot from @Kaawo also had vertical borders. Those are also not present in the Table implementation atm

@Lalaluka Lalaluka requested a review from eeegor as a code owner July 30, 2021 15:15
@Lalaluka
Copy link
Contributor Author

I also noticed some empty rulesets in some of the CSS files.
Are they intended to stay or should they be deleted?

@acstll
Copy link
Collaborator

acstll commented Aug 2, 2021

Thank you @Lalaluka!

I'm checking this out and testing it today, looks perfect at first look.

For the variable name, here is the convention I drafted some time ago but didn't make it in the docs:

  • We use the following naming convention: --{property/thing}-{description/variant}-{state}, e.g. --color-primary-hover.
  • The separation between property/thing, description/variant and state should be evident when reading the variable name.
  • For the property/thing, we may try the token's category/section first (e.g. spacing instead of margin), if it doesn't work or it makes more sense, we use the actual property.
  • Use x for left/right, y for top/bottom properties.

Based on this, I would name it --background-row-striped or --background-tr-striped since there are some variables using th?

Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

I left some comments, the variable name and something minor.

Thank you very much for updating the hbs template and the visual tests!

Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

πŸš€

@Lalaluka Lalaluka requested a review from acstll August 3, 2021 07:05
Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

πŸ‘

@acstll acstll merged commit 9fb0999 into telekom:main Aug 3, 2021
@Lalaluka Lalaluka deleted the striped-tables branch August 3, 2021 09:29
@Kaawo
Copy link

Kaawo commented Aug 4, 2021

Good work. I would like to thank both of you. :)

Kutlovcidenis added a commit that referenced this pull request Aug 4, 2021
* main: (56 commits)
  feat: striped tables (#489)
  test(visual): update snapshots (#494)
  Revert "docs(storybook): separate header from shell" (#493)
  docs(storybook): app-shell and app-header distinction (#471)
  build: build project
  build: build
  test: adapt snapshot
  refactor: functions
  refactor: simplify setIconPositionProp()
  refactor: format; lint
  fix: also allow customized icons
  refactor: make functions more robust
  refactor: adapt snapshots; format; lint
  feat: add media query for high contrast
  feat: storybook aria-translation
  feat: accessibility story toggle group
  fix: colorScheme default value
  feat: change translation features
  feat: add accessibility story to toggle-button
  feat: aria translation
  ...
@acstll acstll mentioned this pull request Aug 11, 2021
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