-
Notifications
You must be signed in to change notification settings - Fork 145
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
feat: [Table] enhancements #2322
base: master
Are you sure you want to change the base?
Conversation
|
✅ PR title follows Conventional Commits specification. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 57a5f46:
|
Hi @saurabhdaware @snitin315 @chaitanyadeorukhkar can you please review this PR? It's been a while raising this PR. Thank you |
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.
Hi, sorry we missed reviewing this. Will get it reviewed and merged this week. Added one small comment meanwhile. Will check storybook locally in some time.
* The contentAlign prop can be 'left', 'center', or 'right'. | ||
* The default value is `left`. | ||
**/ | ||
contentAlign?: 'left' | 'center' | 'right'; |
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.
Maybe we can name this textAlign
? since that is a more intuitive CSS prop for this?
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.
done 👍
@anuraghazra So the thing is with multiline cell, the editable inputs will break since inputs can only be of defined sizes. Lets discuss this with design once. @Sumitmaithani will keep you posted. Sorry for the slow reviews as it needs some discussions |
Hey @Sumitmaithani, we discussed this issue (#2322 (comment)) with our design team So ideally we will have to change the height of inputs in TableEditableCell to match the full height similar to excel sheets. Since this is a bit complex change based on how our BaseInput is structured. I think its better to keep this PR for textAlign only and skip the allowMultiline changes for now. We can pick it up later in the team or you can attempt it as separate PR if you're interested. Let me know if this makes sense. |
const _TableCell = ({ | ||
children, | ||
allowMultiline, | ||
textAlign, |
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.
We'll also need textAlign on TableHeaderCell
Description
Fixes : #2319
Changes
added two new props
Additional Information
Component Checklist