Skip to content

Conversation

JKMarkowski
Copy link
Contributor

Related Issue

part of SAP/fundamental-ngx#4622
part of https://github.tools.sap/dxp/jukebox/issues/420

Description

There are

  • added no-data mode
  • responsvie mode borders removed
  • word break css included into cells

Please check whether the PR fulfills the following requirements

  1. The output matches the design specs
  • All values are in rem
  • Text elements follow the truncation rules
  1. The code follows fundamental-styles code standards and style
  • only one top level fd-* class is used in the file
  • BEM naming convention is used
  • Mixins are used for repeatable code (fd-rtl, fd-ellipsis, fd-flex, fd-selected, fd-focus, ect.)
  • A11y support - keyboard support, screenreader support, proper ARIA attributes, etc.
  • fd-reset() mixin is applied to all elements
  • Variables are used, if some value is used more than twice.
  • Checked if current components can be reused, instead of having new code.
  1. Testing
  • tested Storybook examples with "CSS Resources" normalize option
  • tested Storybook examples with "CSS Resources" unnormalize option
  • Verified all styles in IE11
  • Updated tests
  • last commit message should have [ci visual] so it can trigger chromatic visual regression (e.g. test: run chromatic visual regression [ci visual])
  1. Documentation
  • Storybook documentation has been created/updated
  • Breaking Changes wiki has been updated in case of breaking changes.

@JKMarkowski JKMarkowski added this to the Sprint 57 - Edinburgh milestone Mar 10, 2021
@JKMarkowski JKMarkowski requested a review from a team March 10, 2021 13:41
@JKMarkowski JKMarkowski self-assigned this Mar 10, 2021
@JKMarkowski JKMarkowski changed the title feat: Add no-data, remove borders from responsive feat: Add no-data, remove borders from responsive mode Mar 10, 2021
@netlify
Copy link

netlify bot commented Mar 10, 2021

Deploy preview for fundamental-styles ready!

Built with commit e4a7075

https://deploy-preview-2192--fundamental-styles.netlify.app

@JKMarkowski JKMarkowski force-pushed the feat/table-enhancements branch from 7f4b0a9 to 27d97d3 Compare March 10, 2021 13:45
Copy link
Contributor

@InnaAtanasova InnaAtanasova left a comment

Choose a reason for hiding this comment

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

Screen Shot 2021-03-10 at 10 34 30 AM

Screen Shot 2021-03-10 at 10 34 13 AM

word-break: break-all will break the overflow rules. There might be cases where the user will want to add ... and not break the words

@InnaAtanasova
Copy link
Contributor

Screen Shot 2021-03-10 at 10 34 30 AM Screen Shot 2021-03-10 at 10 34 13 AM

word-break: break-all will break the overflow rules. There might be cases where the user will want to add ... and not break the words

this might be an option when max-width of the cell is provided

@JKMarkowski JKMarkowski force-pushed the feat/table-enhancements branch from 58ca285 to 48a428e Compare March 12, 2021 09:00
@JKMarkowski JKMarkowski force-pushed the feat/table-enhancements branch from 6028743 to d085462 Compare March 12, 2021 09:56
@JKMarkowski JKMarkowski force-pushed the feat/table-enhancements branch from 7c6bfa0 to cbf53f7 Compare March 12, 2021 10:35
@JKMarkowski
Copy link
Contributor Author

JKMarkowski commented Mar 12, 2021

@InnaAtanasova changes that we agreed on are applied, could you check it again? :)

Comment on lines +128 to +130
word-break: break-word;
overflow: hidden;
text-overflow: ellipsis;
Copy link
Contributor

Choose a reason for hiding this comment

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

you could use the mixin here

Copy link
Contributor

Choose a reason for hiding this comment

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

@itmilos why did you resolve this comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

One of the Definitions of Done is Mixins are used for repeatable code (fd-rtl, fd-ellipsis, fd-flex, fd-selected, fd-focus, ect.) and there's a mixin fd-ellipsis that has these 3 rules

@JKMarkowski JKMarkowski merged commit c2ad842 into main Mar 15, 2021
@JKMarkowski JKMarkowski deleted the feat/table-enhancements branch March 15, 2021 09:16
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