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: table updates to match UI Kit v1.4.3 #1501

Merged
merged 8 commits into from
Jun 26, 2019
Merged

Conversation

saad-mo
Copy link
Contributor

@saad-mo saad-mo commented Jun 21, 2019

Closes #1500

adds semantic highlighting support for table rows

Screen Shot 2019-06-21 at 10 28 03 AM

Test

Changelog

New

  • fd-table__row--valid, fd-table__row--warning, fd-table__row--error and fd-table__row--information modifiers for semantic row highlighting

Changed

nothing

Removed

nothing

@saad-mo saad-mo requested a review from a team June 21, 2019 15:31
@saad-mo saad-mo added this to the sprint 13 - Hot Pie milestone Jun 21, 2019
@netlify
Copy link

netlify bot commented Jun 21, 2019

Deploy preview for fundamental ready!

Built with commit 9f389c3

https://deploy-preview-1501--fundamental.netlify.com

Copy link

@LeoT7508 LeoT7508 left a comment

Choose a reason for hiding this comment

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

  1. The Column Header bar should be 40px tall.

  2. The color strip on the left of the rows should have 4 px rounded corners on top and bottom left corners.

  3. Can we also double check that the link text in the rows is the correct Action 1 color? It doesn't look right.

  4. Can we confirm that the spacing from the colored marker and the check box is 12px?

@saad-mo
Copy link
Contributor Author

saad-mo commented Jun 24, 2019

  1. The Column Header bar should be 40px tall.
  2. The color strip on the left of the rows should have 4 px rounded corners on top and bottom left corners.
  3. Can we also double check that the link text in the rows is the correct Action 1 color? It doesn't look right.
  4. Can we confirm that the spacing from the colored marker and the check box is 12px?

@LeoT7508 all changes are posted, pls review again - https://deploy-preview-1501--fundamental.netlify.com/components/table.html

Copy link

@LeoT7508 LeoT7508 left a comment

Choose a reason for hiding this comment

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

Looks like the row heights are 50px, they should be 60px.

Can we also make sure that link text is the right color? (0A6ED1)

@bcullman
Copy link
Contributor

@LeoT7508 - this sounds like scope creep. This PR is about adding highlight support for table rows.

Either this PR needs to be retitled, or non-semantic color work needs to happen in another PR.

@saad-mo
Copy link
Contributor Author

saad-mo commented Jun 25, 2019

@LeoT7508 - this sounds like scope creep. This PR is about adding highlight support for table rows.

Either this PR needs to be retitled, or non-semantic color work needs to happen in another PR.

This is part of the overall tables updates. I will name the PR to reflect the full scope of the changes.

@saad-mo saad-mo changed the title feat: add semantic highlighting support for table rows feat: table updates to match UI Kit updates Jun 25, 2019
@saad-mo saad-mo changed the title feat: table updates to match UI Kit updates feat: table updates to match UI Kit v1.4.3 Jun 25, 2019
@LeoT7508
Copy link

@saad-mo

Cool, we can handle those other pieces later.

@saad-mo
Copy link
Contributor Author

saad-mo commented Jun 25, 2019

@LeoT7508

I looked into the row height. The default height is 52px which is a result of 16px top and bottom padding plus 20px for text. This height changes depending on the which component is added to one of the table cells. For example, row height changes to 56px if you have a small avatar (fd-image--s) and 69px when you add a contextual menu button. I think we should not force a fixed height of 60px on the rows as it will lead to layout issues in more complex scenarios.

if you agree, pls approve the PR.

@LeoT7508
Copy link

@saad-mo

I don't know if that totally makes sense. Because if we have different elements in rows then the row heights will vary and the tables will look broken. What issues will we run into with more complex scenarios? I prefer having a consistent design vs something that could vary and not look right.

@jeannewalters
Copy link
Member

jeannewalters commented Jun 26, 2019

@LeoT7508 @saad-mo I think more complex scenarios are when values wrap in columns when data is longer for some lines v others. I can definitely see where this might be needed,

@LeoT7508
Copy link

@jeannewalters

We have dealt with those, but we should make the base size 60 always unless we have 2 lines of text.

@saad-mo
Copy link
Contributor Author

saad-mo commented Jun 26, 2019

We have dealt with those, but we should make the base size 60 always unless we have 2 lines of text.

The cell height is controlled by padding. We can set to 60px for sure plain text, but the height will increase if we add any component, for example, an identifier or a button. and we will quickly run into verticle alignment issues if we set a fixed height of 60px

let's get on a call. I can explain the issue.

Copy link

@LeoT7508 LeoT7508 left a comment

Choose a reason for hiding this comment

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

Changes look good, padding inside table rows make sense.

@saad-mo saad-mo merged commit b1d721c into master Jun 26, 2019
@saad-mo saad-mo deleted the feature/1500-table-update branch June 26, 2019 19:49
droshev pushed a commit that referenced this pull request Jul 16, 2020
* add semantic highlighting support for table rows
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.

Lib: Update table based on UI Kit updates
4 participants