CustomSelectControl: Deprecate constrained width style #43230
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are no CustomSelectControls being used in here anymore
mirka
added
[Package] Components
mirka
requested review from
draganescu,
talldan,
tellthemachines,
adamziel,
kevin940726,
getdave and
ellatrix
as code owners
mirka
changed the title
mirka
added
the
Needs Dev Note
mirka
force-pushed
the
remove-custom-select-width-hacks
branch
from
1e2ba84
to
2d9bbae
Compare
mirka
commented
Aug 15, 2022
Comment on lines
-32
to
-35
| .components-custom-select-control__button { | ||
| width: 100%; | ||
| } | ||
|
|
Comment on lines
-108
to
-111
| .components-custom-select-control, | ||
| .components-custom-select-control__button { | ||
| width: 100%; | ||
| } |
There was a problem hiding this comment.
This package does not use the CustomSelectControl component anymore.
|
Size Change: -39.1 kB (-3%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ciampo
approved these changes
Aug 16, 2022
There was a problem hiding this comment.
LGTM 🚀
- ✅ Component looks good in Storybook
- ✅ DateFormatPicker looks good in the editor
- ✅ All components using
CustomSelectControl(including parent components forwarding props to it) have the prop set correctly - ✅ Couldn't find any additional CSS style overrides that were "hacking" the width
Couple of additional things that I noticed while reviewing this PR:
- It looks like this classname isn't used anymore, maybe we could remove it?
- Since
FontAppearanceControlsets the prop internally, should we remove this unnecessary prop setting in the typography panel ?
|
Thanks for the review!
Given the guidelines that were pointed out yesterday, I'm inclined to reduce the number of Dev Notes we have to publish. So like, keep the classname if it's easy to keep, and when it's time to remove the classnames in a component (e.g. full Emotion refactor), remove them all in one go so we only have to publish one Dev Note for it. What do you think?
Fixed 👍 833a131 |
Sounds good, feel free to merge when CI is ✅ |
aristath
approved these changes
Aug 16, 2022
|
Hi, noob question: what is this |
@draganescu Good question! This is a convention we've started using in the Components package to deal with style deprecations, as documented here in our package contributing guide. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Formally deprecate the 130px min-width style on CustomSelectControl.
All in-repo usage has been migrated, and style hacks have been removed.
Why?
For easier hack-free styling, and consistency across components.
Testing Instructions
CustomSelectControl
__nextUnconstrainedWidthprop set to true.DateFormatPicker
✍️ Dev Note
To improve consistency with
SelectControl, the CSS styles forCustomSelectControlwill be changed to take up the full width available, regardless of how short the option strings are. To start opting into these new unconstrained width styles, set the__nextUnconstrainedWidthprop totrue. These styles will become the default in 6.4.To keep your
CustomSelectControlsimilar to the previous styling, add width constraints to a wrapper div.