-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix datatable layout #6108
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 layout #6108
Conversation
package.json
Outdated
@@ -86,7 +86,7 @@ | |||
"babel-core": "^7.0.0-bridge.0", | |||
"babel-jest": "^27.5.1", | |||
"babel-loader": "^8.2.5", | |||
"babel-plugin-styled-components": "^2.0.7", | |||
"babel-plugin-styled-components": "2.0.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no I have it in another PR Ill strip this out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was strange changes in snapshots made so I opened this PR
#6110
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
taken out
yarn.lock
Outdated
@@ -3541,7 +3541,18 @@ babel-plugin-react-docgen@^4.2.1: | |||
lodash "^4.17.15" | |||
react-docgen "^5.0.0" | |||
|
|||
"babel-plugin-styled-components@>= 1.12.0", babel-plugin-styled-components@^2.0.7: | |||
babel-plugin-styled-components@2.0.6: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yarn.lock shouldn't change either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed it got merged into master
…datatable-layout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I agree we should update the size
prop description in the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
What does this PR do?
This PR takes out
This was setting the
table-layout
to be fixed in firefox which then the table’s layout ignores the content and instead uses the table’s width, any specified width of columns, and border and cell spacing values.Where should the reviewer start?
styledTable.js
What testing has been done on this PR?
storybook
How should this be manually tested?
Use the code above in a story and see that in FireFox it was a fixed width for each cell which should not be the right behavior without setting
size
we want the cells to grow with the contentDo Jest tests follow these best practices?
screen
is used for querying.userEvent
is used in place offireEvent
.asFragment()
is used for snapshot testing.Any background context you want to provide?
The
was put in as a fix for
If
Table
is wrapped in (Box with a width limit) and is inside this table, then in Firefox it will go beyond the limits of this Box. The problem is found only in Firefox - in Chrome and in Safari everything works fine.#2656
FireFox was not respecting the width however this was 3 years ago and things have changed this is no longer an issue and FF respects the width that is inherited.
This can be tested with the code below in FireFox
What are the relevant issues?
closes #6081
Screenshots (if appropriate)
Do the grommet docs need to be updated?
Im thinking we should update the
size
prop for Datatable in the docs it states now it only effects the height however whensize
is set it addsShould this PR be mentioned in the release notes?
sure
Is this change backwards compatible or is it a breaking change?
backwards compatible