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): expand + select padding #8656

Merged
merged 8 commits into from
May 12, 2021

Conversation

jnm2377
Copy link
Contributor

@jnm2377 jnm2377 commented May 11, 2021

Closes #8472

Testing / Reviewing

Make sure the expandable + select example has updated padding:
- expand button should be 32px wide now (with an additional 8px left padding in the parent cell)
- selectable cell should also be 32px wide
- the table cell following the select should have 8px left padding now
- select and expand button focus should now match sizes

@jnm2377 jnm2377 requested a review from aagonzales May 11, 2021 19:18
@jnm2377 jnm2377 requested a review from a team as a code owner May 11, 2021 19:18
Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

@netlify
Copy link

netlify bot commented May 11, 2021

Deploy preview for carbon-elements ready!

Built with commit 0211ab4

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

@netlify
Copy link

netlify bot commented May 11, 2021

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 0211ab4

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

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

The spacing looks perfect now!

Just wondering if can we update the focus box height for the expansion chevron to be 32px tall for size small and up? It feels weird to tab through the tall chevron focus to the small checkbox focus. I know checkbox focus is coming from the component probably but it would be nice for visual consistency in tabbing if those focuses were the same. Also let me know if i should just open a new issue for this.

image

@jnm2377 jnm2377 requested a review from aagonzales May 11, 2021 22:09
@jnm2377
Copy link
Contributor Author

jnm2377 commented May 11, 2021

@aagonzales updated 👍🏽

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Does the checkbox need more padding-right, or the text more padding-left? The checkbox doesn't look centered between the expansion chevron and the text like it does in the spec from the issue

Screen Shot 2021-05-12 at 10 05 01 AM
Screen Shot 2021-05-12 at 10 05 18 AM

@aagonzales
Copy link
Member

aagonzales commented May 12, 2021

Oh yeah good catch @tay1orjones looks like its 4px off. I think just add those 4px to padding-right of checkbox? Checkbox padding is alway hard because the checkbox asset we use in code is different than the asset (svg) we use in design files.

Top row code, bottom row design spec
image

image

@jnm2377
Copy link
Contributor Author

jnm2377 commented May 12, 2021

@tw15egan @tay1orjones @aagonzales updated! I added a knob to the Development > example story so you can test the expansion + select spacing/focus in different sizes.

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Ship it!

@kodiakhq kodiakhq bot merged commit 76f3c62 into carbon-design-system:main May 12, 2021
@tw15egan tw15egan mentioned this pull request May 13, 2021
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data table: expand + select spacing
4 participants