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): fix small toolbar sizing and color bug #10296

Merged
merged 12 commits into from
Jan 31, 2022

Conversation

jnm2377
Copy link
Contributor

@jnm2377 jnm2377 commented Dec 15, 2021

Closes #10058

Changelog

  • cleans up data table stories in v10:
    • deletes DataTable-story.js since storybook wasn't using those stories
    • adds basic table examples (w/ sizes + zebra)
    • adds toolbar examples (w/ different sizes)
    • adds small batch actions example
    • renamed some examples to be more semantic
  • adds v11 data table stories which were missing
  • fixes small toolbar sizing issues
  • fixes batch action sizing issues
  • fixes "color" issues in overflow menu: OverflowMenu v11 refactor wasn't adding the icon class name to the trigger icon, so it wasn't getting icon fill theming, which was also present in DataTable

Testing / Reviewing

  • View v10 and v11 toolbar examples to check styles are correct. We don't currently have a story that shows the bug, so can checkout locally and comment out the new styles to see what the existing bug looks like, if you need a reference on what to look for.
  • Test out different theme colors for v11 DataTable, and check to see that overflow menu icon fill is now changing color according to theme

@jnm2377 jnm2377 requested a review from a team as a code owner December 15, 2021 18:44
@jnm2377 jnm2377 requested review from aledavila and dakahn December 15, 2021 18:44
@netlify
Copy link

netlify bot commented Dec 15, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: cf8a596

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61f85814943dea000879490d

😎 Browse the preview: https://deploy-preview-10296--carbon-react-next.netlify.app

@jnm2377 jnm2377 changed the title fix(DataTable fix(DataTable): fix small toolbar sizing and color bug Dec 15, 2021
@jnm2377 jnm2377 marked this pull request as draft December 15, 2021 18:50
@netlify
Copy link

netlify bot commented Dec 15, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: cf8a596

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61f858141fc448000844a8b5

😎 Browse the preview: https://deploy-preview-10296--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Dec 15, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: cf8a596

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61f858150b299400089895b8

😎 Browse the preview: https://deploy-preview-10296--carbon-components-react.netlify.app

@jnm2377 jnm2377 marked this pull request as ready for review December 15, 2021 19:49
@abbeyhrt
Copy link
Contributor

@jnm2377 oof, the data table stories were so chaotic, thank you for taking that on and re-organizing so it worked with @carbon/react! I like the new arrangement overall but could we have some stories that aren't under folders? Just maybe like the Usage story or "Playground" story

@abbeyhrt
Copy link
Contributor

Couple of spacing issues with the toolbar in @carbon/react but the v10 one looks all good!

Screen Shot 2021-12-16 at 2 15 43 PM
Screen Shot 2021-12-16 at 2 15 25 PM
Screen Shot 2021-12-16 at 2 15 17 PM

@jnm2377
Copy link
Contributor Author

jnm2377 commented Jan 11, 2022

@abbeyhrt updated the bugs you found 👍🏽

@abbeyhrt
Copy link
Contributor

Nice! I think it looks mostly good but, at least comparing the v11 data table to v10, it looks like there is a little too much space to the left of the icon here:
v11:
Screen Shot 2022-01-13 at 2 58 36 PM

v10:
Screen Shot 2022-01-13 at 3 00 28 PM

@jnm2377
Copy link
Contributor Author

jnm2377 commented Jan 27, 2022

@abbeyhrt and @tay1orjones updated the styles, so v11 toolbar search icon should be aligned now. Sorry the PR files might be a little confusing with all the story changes. The fixes pertaining to this bug were really in 3 files:

  • packages/components/src/components/data-table/_data-table-action.scss
  • packages/styles/scss/components/data-table/action/_data-table-action.scss
  • packages/react/src/components/OverflowMenu/next/OverflowMenu.js

The rest are story updates.

@jnm2377 jnm2377 enabled auto-merge (squash) January 27, 2022 22:12
@tay1orjones
Copy link
Member

Checks were failing due to updates pulled in from #10535. I pushed an update converting $layer-disabled to use $layer instead 👍

Copy link
Contributor

@abbeyhrt abbeyhrt left a comment

Choose a reason for hiding this comment

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

Looks great!!

@jnm2377 jnm2377 merged commit 60d7518 into carbon-design-system:main Jan 31, 2022
@jnm2377 jnm2377 mentioned this pull request Feb 10, 2022
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.

[Bug]: TableToolbarMenu size and color incorrect
3 participants