-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Automattic Components: Add select dropdown to package #79255
Conversation
9ff9602
to
9d05080
Compare
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
0eafb8f
to
d0e6432
Compare
8f99aba
to
5ef0c35
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.
Had a minor concern but I don't think there is an easy way to resolve it other than doing everything in one go.
Works well, LGTM 🚢
@@ -0,0 +1,236 @@ | |||
# Select Dropdown |
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.
The only contention I have with this issue is that we loose the blame history of the file once we delete that.
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.
Ah this is a super interesting point that I also don't have a great answer to. For me, being able to break down the migration into easily digestible PRs is worth the trade off, especially for such a wide reaching component.
At the very least though, I'll go back and make a note in the pull request descriptions for this PR, #79256, and #79257. That way, if anyone else follows this same process, they'll be aware of the tradeoffs from the get-go. If you think of any other suggestions, feel free to let me know and we can try and implement them 👍
Proposed Changes
I understand that these seem like a large volume of changes, but we're simply cloning the existing
client/components/
select dropdown component into@automattic/components
as we did in #79257 and #79256The entirety of the migration is a 4 step process that creates easily digestible ( and revertible ) PRs to work with. See https://github.com/Automattic/martech/issues/2028. Unfortunately, this process also eliminates the git history from migrated files unless the 4 steps are condensed into a single one. I don't have a great answer to this, but the tradeoff, to me, is worth the added safety of more understandable, compact, iterative pull requests.
GIFs
Testing Instructions
yarn run test-packages packages/components/src/select-dropdown/test
if you'd like to run it locally, but the CI process should catch anything.yarn storybook:start
Pre-merge Checklist