Skip to content

fix(react): adjust bx--actions-list in data table to use display grid instead of position absolute#9994

Merged
abbeyhrt merged 16 commits into
carbon-design-system:mainfrom
abbeyhrt:fix/9686
Nov 4, 2021
Merged

fix(react): adjust bx--actions-list in data table to use display grid instead of position absolute#9994
abbeyhrt merged 16 commits into
carbon-design-system:mainfrom
abbeyhrt:fix/9686

Conversation

@abbeyhrt

@abbeyhrt abbeyhrt commented Oct 28, 2021

Copy link
Copy Markdown
Contributor

Closes #9686

Updates how bx--actions-list is positioned so that whether or not it needs to scroll is sensed as expected. Because it was positioned absolutely, it didn't show the scroll bar when the items broke out of their parent container, this addresses that and adjusts the styles where needed.

Changelog

New

  • An example for a data table with lots of batch actions for testing, will be removed before merge.

Changed

  • updates the action list to display grid to right align with buttons without position: absolute needed

Removed

  • Redundant or unnecessary styles.

Testing / Reviewing

I added a story to DataTable w/ Batch Actions that has a ton of actions, shrink your screen until some aren't visible and make sure that the horizontal scroll bar appears and you can scroll to all the items.

The new story will be removed before merge.

@abbeyhrt abbeyhrt requested a review from a team as a code owner October 28, 2021 21:41
@abbeyhrt abbeyhrt requested a review from dakahn October 28, 2021 21:41
@netlify

netlify Bot commented Oct 28, 2021 •
edited
Loading

Copy link
Copy Markdown

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

🔨 Explore the source changes: bc51d17

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

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

@netlify

netlify Bot commented Oct 28, 2021 •
edited
Loading

Copy link
Copy Markdown

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: bc51d17

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

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

@netlify

netlify Bot commented Oct 28, 2021 •
edited
Loading

Copy link
Copy Markdown

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

🔨 Explore the source changes: bc51d17

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

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

@joshblack joshblack requested a review from aagonzales October 29, 2021 15:21
aagonzales
aagonzales suggested changes Oct 29, 2021
View reviewed changes

@aagonzales aagonzales left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The scroll works good but the summary is getting cut off on the left side.

image

We could keep the summary and have the other item scroll under it OR we can just have the summary scroll with the actions.

image

@abbeyhrt abbeyhrt requested a review from aagonzales November 1, 2021 18:37
tay1orjones
tay1orjones approved these changes Nov 2, 2021
View reviewed changes

@tay1orjones tay1orjones left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me 👍

When you hover over a button that is behind the summary, you can see that the summary isn't as tall as the buttons.
Screen Shot 2021-11-01 at 10 05 44 PM
Would be cool if the summary could be the same height as the buttons, but that might be a limitation of the layout configuration happening here. In general this is a huge improvement as-is even without that piece.

@abbeyhrt

abbeyhrt commented Nov 3, 2021

Copy link
Copy Markdown
Contributor Author

@aagonzales Good catch! I fixed the scroll issue you pointed out and adjusted the height that Taylor mentioned, would you mind doing a quick re-review?

aagonzales
aagonzales approved these changes Nov 3, 2021
View reviewed changes

@aagonzales aagonzales left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!

@abbeyhrt abbeyhrt enabled auto-merge (squash) November 3, 2021 19:17
@abbeyhrt abbeyhrt disabled auto-merge November 3, 2021 19:58
@abbeyhrt abbeyhrt enabled auto-merge (squash) November 3, 2021 20:05
@abbeyhrt abbeyhrt merged commit 963eb58 into carbon-design-system:main Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tay1orjones tay1orjones tay1orjones approved these changes

+3 more reviewers

@joshblack joshblack joshblack approved these changes

@aagonzales aagonzales aagonzales approved these changes

@dakahn dakahn dakahn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status: ready to merge 🎉

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: DataTable Batch Actions cut off on smaller screens

5 participants

@abbeyhrt @tay1orjones @joshblack @aagonzales @dakahn