BorderBoxControl: Refactor stories to TypeScript and Controls #45002
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
|
Size Change: 0 B Total Size: 1.28 MB ℹ️ View Unchanged
|
ciampo
reviewed
Oct 18, 2022
There was a problem hiding this comment.
Thank you Aaron, this is definitely an improvement over the current Storybook examples, and a great way to have more insights on this component's docs and APIs.
In fact, as I reviewed this PR, I noticed a few inconsistencies that should be probably be ironed out:
- most props lack a default value. While this is not a mandatory, I think it would make the docs much more clear and set better expectations on what is the component's default behavior (i.e. are custom colors enabled by default? are border styles enabled by default? etc.)
- I actually noticed a couple of inconsistencies with the docs:
- the
enableStyleprop should show a default value oftrue, since it's forwarded toBorderControlcomponents where that prop has a default value oftrue - the
colorsprop is marked as not required, but in my tests the component breaks when it's omitted (the error actually happens in theColorPalettecomponent) — an easy fix could be to assign a default value of[]
- the
- especially for props that are simply forwarded to other components (like
BorderControlandPopover), we should try to reuse their types (ie. inherit / pick their props when possible) - there could be other inconsistencies that I didn't spot — which highlights how valuable it can be to write Storybook examples and unit tests in TypeScript
I'm ok with fixing these inconsistencies in a follow-up PR, so that we can focus on merging the few PRs on BorderBoxControl that are currently opened.
|
Appreciate the review and detailed explanations @ciampo 👍 As you suggest, I'll create a separate PR to iron out any inconsistencies with prop defaults, types, and documentation. In the meantime, the other feedback for this PR has been addressed so 🤞 it should be right to land when I get a final approval on #41860. |
|
FYI - |
0a48021 to
be2faf1
Compare
Also fixes the duplicate colors that were in the BorderControl's shortened color array.
364c803 to
827ec9b
Compare
|
Update — #44632 has been merged (in case you were going to open that follow-up on ironing out |
aaronrobertshaw
added a commit
that referenced
this pull request
Oct 26, 2022
ciampo
added a commit
that referenced
this pull request
Oct 26, 2022
* Update border control components to allow 40px height * Make 40px height work again after rebase * Turn on 40px tall border controls in editors * Update radius control to handle extra padding These tweaks update the BorderRadiusControl such that the entered values are actually visible despite the increased internal padding applied to the UnitControl when the `__unstable-large` variant is used. * First pass at improving border controls when at 40px Initial tweaks to improve the UX for the border controls. Aims to address the increased padding for large 40px high UnitControl variant that cuts of entered values from display. * Make compact inputs 118px wide * Increase padding to 16px between input and slider * Make split controls gap a uniform 16px * Update split layout for floating linked button * Make color dropdown 40px square with 16px indicator * Tweak input widths again to fit editor sidebar * Make radius control match border and grid * Remove hardcoded width from radius input wrapper * Try using Mix as placeholder for mixed values * Revert placeholder back to 'Mixed' * Remove use of internal component classnames * Remove unnecessary flex styles from linked button * Use space util for color swatch dimensions * Add explanatory comment for compact widths * Better align the left border for 30px height variant * Consolidate border radius controls to single 40px height This allows us to remove most of the changes that would have been introduced in this PR as well as simplify the CSS. * Fix RTL rendering and style naming * Remove unneeded prop setting 40px height * Switch __next40pxDefaultSize to size __unstable-large * Fix up defaults in types and update READMEs * Add components changelog * Rename new prop `__unstableWrapperClassName` * UnitControl: Normalize wrapper classnames by removing outer wrapper (#45139) * BorderBoxControl: Refactor stories to TypeScript and Controls (#45002) * Remove max-width wrappers from border control stories Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
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.
Part of: #35744
What?
Finishes off conversion of BorderBoxControl to TypeScript, including updating stories to use controls instead of knobs.
Why?
How?
BorderBoxControlnamed component so the type data can be extracted.BorderBoxControlas non-polymorphic.Testing Instructions
npm run storybook:devBorderBoxControlstory still function as expected