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.

Sign up for GitHub

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

Jump to bottom

feat: [Table] enhancements #2322

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

feat: [Table] enhancements #2322

wants to merge 5 commits into from
+37 −8

Conversation

Sumitmaithani
Copy link
Contributor

Description

Fixes : #2319

Changes

added two new props

  /**
   * The contentAlign prop determines the content alignment of the table.
   * The contentAlign prop can be 'left', 'center', or 'right'.
   * The default value is `left`.
   **/
  contentAlign?: 'left' | 'center' | 'right';
  /**
   * The allowMultiline prop determines whether the table should allow multiline cells or not.
   * The default value is `false`.
   **/
  allowMultiline?: boolean;

Additional Information

Component Checklist

  • Update Component Status Page
  • Perform Manual Testing in Other Browsers
  • Add KitchenSink Story
  • Add Interaction Tests (if applicable)
  • Add changeset

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 27, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: 57a5f46

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Sumitmaithani Sumitmaithani marked this pull request as draft July 27, 2024 12:23
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 27, 2024 •
edited
Loading

✅ PR title follows Conventional Commits specification.

@codesandbox-ci CodeSandbox CI
Copy link

codesandbox-ci bot commented Jul 27, 2024 •
edited
Loading

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:

Sandbox Source
razorpay/blade: basic Configuration

@Sumitmaithani Sumitmaithani marked this pull request as ready for review July 28, 2024 10:53
@Sumitmaithani
Copy link
Contributor Author

Hi @saurabhdaware @snitin315 @chaitanyadeorukhkar can you please review this PR? It's been a while raising this PR. Thank you

saurabhdaware
saurabhdaware reviewed Aug 26, 2024
View reviewed changes
Copy link
Member

@saurabhdaware saurabhdaware left a 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.

packages/blade/src/components/Table/types.ts Outdated
* The contentAlign prop can be 'left', 'center', or 'right'.
* The default value is `left`.
**/
contentAlign?: 'left' | 'center' | 'right';
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👍

@saurabhdaware saurabhdaware self-requested a review August 28, 2024 06:25
@saurabhdaware saurabhdaware self-assigned this Aug 28, 2024
@saurabhdaware saurabhdaware added the Review - L2 Second level of review label Aug 30, 2024
@saurabhdaware
Copy link
Member

@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

image

@saurabhdaware
Copy link
Member

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.
image

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.

saurabhdaware
saurabhdaware reviewed Aug 30, 2024
View reviewed changes
packages/blade/src/components/Table/TableBody.web.tsx
const _TableCell = ({
children,
allowMultiline,
textAlign,
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saurabhdaware saurabhdaware saurabhdaware left review comments

@chaitanyadeorukhkar chaitanyadeorukhkar Awaiting requested review from chaitanyadeorukhkar chaitanyadeorukhkar is a code owner

@kamleshchandnani kamleshchandnani Awaiting requested review from kamleshchandnani kamleshchandnani is a code owner

@anuraghazra anuraghazra Awaiting requested review from anuraghazra anuraghazra is a code owner

@snitin315 snitin315 Awaiting requested review from snitin315 snitin315 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@saurabhdaware saurabhdaware

@anuraghazra anuraghazra

Labels
Review - L2 Second level of review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Table]: Few enhancements
3 participants
@Sumitmaithani @saurabhdaware @anuraghazra