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

fix(DataTable): address spacing and alignment issues #6197

Merged

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Jun 4, 2020

Closes #6184

This PR addresses several spacing and alignment issues in the data table:

  • Expansion column style rules should be consistent in all table variants
  • selection (checkbox) cells are now correctly aligned and spaced
  • Changing the size no longer breaks cell height for expansion cells and selection (checkbox) cells
  • radio button selection cells are now aligned and sized according to spec
  • some documentation-only changes to correct the appearance of expanded row content

Testing / Reviewing

Confirm all table sizes and variants appear correct

@netlify
Copy link

netlify bot commented Jun 4, 2020

Deploy preview for carbon-elements ready!

Built with commit 0763bab

https://deploy-preview-6197--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Jun 4, 2020

Deploy preview for carbon-components-react ready!

Built with commit 0763bab

https://deploy-preview-6197--carbon-components-react.netlify.app

@emyarod emyarod force-pushed the 6184-data-table-cell-alignment branch 2 times, most recently from 38b6ebc to e832311 Compare June 8, 2020 17:38
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

with boolean column and with overlfow menu stories:
The column header needs to be the same size as the rows per size prop change.
Tall: 64px/4rem
Default: 48px/3rem
Short: 32px/2rem
Compact: 24px/1.5rem

Example of what is happening:
Screen Shot 2020-06-08 at 2 35 54 PM

Expandable row stories:
-Can you bring the chevron icons down a pixel for stories that have expansion? They seem too high.
-When expanded, there is a line that is flashing next to the expansion rule divider. This is only happening in Firefox but I don’t know if it is possible to get it to stop doing that.

Radio button story:
The radio buttons in each row need to come down a pixel. They look too high.

@laurenmrice
Copy link
Member

laurenmrice commented Jun 8, 2020

Here is a data table spec for all sizes with padding specified:

datable

@emyarod
Copy link
Member Author

emyarod commented Jun 8, 2020

the row height issues for those 2 examples in particular are because of the sizes of the checkbox and overflow menu trigger button. unless it's alright to scale those down in size, the rows need to expand a bit to allow those elements to fit

@laurenmrice
Copy link
Member

laurenmrice commented Jun 9, 2020

@emyarod Why do the icons require extra space which would make the rows larger? Is it the states applied to those icons that is causing the issue?

This is an example of our original spec of the compact and short data table when states are applied. The icons are smaller than the rows.

Compact:
compact

Short:
short

@emyarod
Copy link
Member Author

emyarod commented Jun 9, 2020

for the overflow menu, it looks like it's taken directly from the OverflowMenu component which has a 32x32 trigger button, but if it's fine to reduce the height then I'll add rules for that

for the checkbox, the component is 24px tall but the padding on every table cell expands the row height. I do have a solution in mind but it might be a little hacky

but since neither of these are regressions I can tackle them separately (my original ticket was for text alignment) but if you think it should be rolled together I can do that too

@emyarod emyarod force-pushed the 6184-data-table-cell-alignment branch from a0313b7 to 2122113 Compare June 9, 2020 15:07
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

  • “with batch actions”, “with overflow menu” and “with selection” stories:
    -The first checkbox in the column header is too low for the default size.
    checkbox

  • “with batch expansion”, “with dynamic content” and “with expansion” stories:
    -The chevrons are too high for all sizes. Need to come down 1px.
    chevrons

  • “with radio button selection” story:
    -The radio button is too high in each row for the tall size.
    radiobutton

@emyarod emyarod force-pushed the 6184-data-table-cell-alignment branch from 2122113 to 626cdfc Compare June 9, 2020 17:51
@emyarod emyarod requested a review from laurenmrice June 9, 2020 17:52
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Approving because the last changes about icon control alignments have been fixed. We will tackle row and header heights for the overflow icon and checkbox in another pr. 👍🏻

@emyarod emyarod force-pushed the 6184-data-table-cell-alignment branch from b655a14 to 0763bab Compare June 11, 2020 18:31
@emyarod emyarod merged commit 946680c into carbon-design-system:master Jun 12, 2020
@emyarod emyarod deleted the 6184-data-table-cell-alignment branch June 12, 2020 14:05
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.

data table height bugs
5 participants