-
Couldn't load subscription status.
- Fork 3
feat(Table) add size option on cells
#256
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
base: main
Are you sure you want to change the base?
feat(Table) add size option on cells
#256
Conversation
006e4e3 to
804455b
Compare
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.
I wonderedl for a while if it was not CSS customization realm, but without this option it would hard to have to columns of the same type having different width.
We could argue that columns of the same type should have the same width though—in which case the type class is sufficient as a selector. But overall, I don't see any drawback to adding it, it's optional anyways if someone doesn't want to use it.
| } | ||
| :global(.ods-dataviz--default td.table-data--long-text > span) { | ||
| :global(.ods-dataviz--default td.table-data--long-text > span), | ||
| :global(.ods-dataviz--default td.table-data--geo > div) { |
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 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.
With the changes made to the max-width properties, the short-text and long-text columns look identical in the display of their content. To see a difference, the optional size property needs to be applied.
I think we can improve this system.
Applying a maximum width to certain types of columns seems to be the right approach.
| desc: string; | ||
| }; | ||
| onClick?: () => void; | ||
| size?: 'large' | 'medium' | 'small' | 'fit-content'; |
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.
If this option is not used at all for a table.
There is not difference between using a short-text and a long-text column.
Long texts no longer break on new line as there is no default max-width.
I agree with the comment made by @etienneburdet that default max-width should be set for some column types


Summary
[explo] The goal for this PR is to add an option (size) to define the width of columns.
(Internal for Opendatasoft only) Associated Shortcut ticket: sc-48043.
Changes
New
sizeoption for cell with predefined values (small,medium,largeandfit-content[default])Breaking Changes
none
Changelog
In the Table component: It's now possible to define a size for the column width
Open discussion
explo ... all comments are welcome
To be tested
in storybook
Review checklist