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.

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: Add Table component #1797

Merged
merged 128 commits into from
Nov 24, 2023
Merged

feat: Add Table component #1797

merged 128 commits into from
Nov 24, 2023

Conversation

chaitanyadeorukhkar
Copy link
Collaborator

@chaitanyadeorukhkar chaitanyadeorukhkar commented Nov 7, 2023

Adds Table Component

Documentation: #1834

Copy link

changeset-bot bot commented Nov 7, 2023

🦋 Changeset detected

Latest commit: 7ff6a20

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@razorpay/blade Minor

Not sure what this means? Click here to learn what changesets are.

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

Copy link

codesandbox-ci bot commented Nov 7, 2023

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 7ff6a20:

Sandbox Source
razorpay/blade: basic Configuration

Copy link
Contributor

github-actions bot commented Nov 7, 2023

✅ PR title follows Conventional Commits specification.

@@ -135,6 +136,7 @@
"@babel/preset-react": "7.16.5",
"@babel/preset-typescript": "7.16.5",
"@codesandbox/sandpack-react": "1.16.0",
"@emotion/react": "11.11.1",
Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, does that mean now consumers will have bundle of both styled comp and emotion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup thats a down side of this library. Mentioned it in the API decision 😢

Doesnt add much though

Copy link
Member

@anuraghazra anuraghazra Nov 24, 2023

Choose a reason for hiding this comment

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

I see, so does the library marks @emotion/react as peerDep?

It should, otherwise all the components will have to carry emotion bundle this is what I was exploring also.

Also, let's move @table-library/react-table-library to peerDep, we don't need to include this on our bundle if a consumer doesn't use Table keeping in mind treeshaking/codesplitting things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checking

anuraghazra
anuraghazra previously approved these changes Nov 24, 2023
Copy link
Member

@anuraghazra anuraghazra left a comment

Choose a reason for hiding this comment

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

minor comments

Comment on lines 244 to 245
"@table-library/react-table-library": "4.1.7",
"@emotion/react": "11.11.1",
Copy link
Member

Choose a reason for hiding this comment

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

Another thing we should do that we didn't on other peerDeps, is instead of pinning them we should use ranges, otherwise consumers will have to use the exact version down the the patch otherwise there will be mismatches.

@emotion/react: ^11.11.1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, done!

@chaitanyadeorukhkar chaitanyadeorukhkar merged commit 23a3e11 into master Nov 24, 2023
14 of 15 checks passed
@chaitanyadeorukhkar chaitanyadeorukhkar deleted the feat/table-component branch November 24, 2023 11:04
anuraghazra pushed a commit that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review - L1 First level of review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants