-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
chore: Extract common select component code #21094
chore: Extract common select component code #21094
Conversation
…xtract-common-select-code
Codecov Report
@@ Coverage Diff @@
## master #21094 +/- ##
==========================================
+ Coverage 66.45% 66.56% +0.10%
==========================================
Files 1789 1791 +2
Lines 68296 68411 +115
Branches 7275 7302 +27
==========================================
+ Hits 45387 45535 +148
+ Misses 21034 20995 -39
- Partials 1875 1881 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Hi @cccs-RyanK. Thanks for this! Before I dive into the code I have a general comment. I'd prefer to see a structure similar to this one:
@michael-s-molina what's your thought about 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.
Thank you so much for PR @cccs-RyanK! I left some first-pass comments.
What I generally do is to keep things in context and depending on the size of things, break them into related submodules. One thing I don't like is mixing things that belong to different components. For example, I don't like to have a
If I have a few types or styled-components, I generally define them in the component. |
This makes sense but I still believe we should not have all the shared parts under one utils file even though this is not very big. For instance, I don't see why styles should be under utils. |
Maybe I was not clear but in my example styles would never be under utils. It would belong to the component file if we have a few of them, or to a styles submodule if we have many. So I think we're in agreement. |
…xtract-common-select-code
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. Thanks for all the hard work @cccs-RyanK!
Recently in the ongoing effort to clean up the select component, it was split into Sync and Async components. After splitting they shared alot of common code. This PR is to move some of the shared code into a new common file that each of the components can import from in order to better reuse code.
Overall task is outlined here: #20143
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION