Components: Type BaseControl and VisuallyHidden #26078
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
75 tasks
|
Size Change: 0 B Total Size: 1.21 MB ℹ️ View Unchanged
|
sirreal
force-pushed
the
update/type-more-components
branch
4 times, most recently
from
5bd039d to
3e60029
Compare
sirreal
requested review from
ajitbohra,
chrisvanpatten and
youknowriad
as code owners
sirreal
added
[Package] Components
youknowriad
reviewed
Oct 14, 2020
| @@ -8,6 +8,20 @@ import classnames from 'classnames'; | |||
| */ | |||
| import VisuallyHidden from '../visually-hidden'; | |||
|
|
|||
| /** | |||
| * @typedef Props | |||
There was a problem hiding this comment.
is this type "local"? I mean the name is very generic, is it fine?
There was a problem hiding this comment.
It is local to the module, although it is an exported type. It's pretty common practice to name types like this in my experience.
It doesn't go into any global namespace, but is accessible from another module:
/**
* Make a local Props from the other module Props
* @typedef {import('…').Props} Props
*/
sirreal
commented
Oct 14, 2020
|
I've reverted to a draft. |
sirreal
added
[Status] In Progress
sirreal
force-pushed
the
update/type-more-components
branch
from
e08d399 to
3dba833
Compare
|
I typed utils and this is ready for review again. |
|
I seem to have made a bunch of tests fail so I'll look again 🙂 |
sirreal
force-pushed
the
update/type-more-components
branch
from
06bfe7d to
9732f0c
Compare
sirreal
commented
Oct 16, 2020
| @@ -1,3 +1,3 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|
|||
| exports[`Basic rendering should render with required props 1`] = `"<fieldset class=\\"block-editor-responsive-block-control\\"><legend class=\\"block-editor-responsive-block-control__title\\">Padding</legend><div class=\\"block-editor-responsive-block-control__inner\\"><div class=\\"components-base-control components-toggle-control block-editor-responsive-block-control__toggle css-wdf2ti-Wrapper e1puf3u0\\"><div class=\\"components-base-control__field css-11vcxb9-StyledField e1puf3u1\\"><span class=\\"components-form-toggle is-checked\\"><input class=\\"components-form-toggle__input\\" id=\\"inspector-toggle-control-0\\" type=\\"checkbox\\" aria-describedby=\\"inspector-toggle-control-0__help\\" checked=\\"\\"><span class=\\"components-form-toggle__track\\"></span><span class=\\"components-form-toggle__thumb\\"></span></span><label for=\\"inspector-toggle-control-0\\" class=\\"components-toggle-control__label\\">Use the same padding on all screensizes.</label></div><p id=\\"inspector-toggle-control-0__help\\" class=\\"components-base-control__help css-1wm1a55-StyledHelp e1puf3u3\\">Toggle between using the same value for all screen sizes or using a unique value per screen size.</p></div><div class=\\"block-editor-responsive-block-control__group\\"><div class=\\"components-base-control css-wdf2ti-Wrapper e1puf3u0\\"><div class=\\"components-base-control__field css-11vcxb9-StyledField e1puf3u1\\"><div class=\\"components-flex components-select-control e1cr7zh10 css-xnuudl-Flex-Root eboqfv50\\"><div class=\\"components-flex__item e1cr7zh14 css-d373l0-Item-LabelWrapper eboqfv51\\"><label for=\\"inspector-select-control-0\\" class=\\"components-input-control__label css-za86g6-Text-BaseLabel e1cr7zh13\\"><span aria-describedby=\\"rbc-desc-0\\">All</span><span class=\\"components-visually-hidden\\" id=\\"rbc-desc-0\\">Controls the padding property for All viewports.</span></label></div><div class=\\"components-input-control__container css-nj8rmx-Container e1cr7zh11\\"><select class=\\"components-select-control__input css-hwcz8s-Select e12x0a390\\" id=\\"inspector-select-control-0\\"><option value=\\"\\">Please select</option><option value=\\"small\\">Small</option><option value=\\"medium\\">Medium</option><option value=\\"large\\">Large</option></select><span class=\\"components-input-control__suffix css-1lym30p-Suffix e1cr7zh17\\"><div class=\\"css-s55r1c-DownArrowWrapper e12x0a391\\"><svg viewBox=\\"0 0 24 24\\" xmlns=\\"http://www.w3.org/2000/svg\\" width=\\"18\\" height=\\"18\\" role=\\"img\\" aria-hidden=\\"true\\" focusable=\\"false\\"><path d=\\"M17.5 11.6L12 16l-5.5-4.4.9-1.2L12 14l4.5-3.6 1 1.2z\\"></path></svg></div></span><div aria-hidden=\\"true\\" class=\\"components-input-control__backdrop css-1s1n1to-BackdropUI e1cr7zh15\\"></div></div></div></div></div><p id=\\"all\\">All is used here for testing purposes to ensure we have access to details about the device.</p></div></div></fieldset>"`; | |||
| exports[`Basic rendering should render with required props 1`] = `"<fieldset class=\\"block-editor-responsive-block-control\\"><legend class=\\"block-editor-responsive-block-control__title\\">Padding</legend><div class=\\"block-editor-responsive-block-control__inner\\"><div class=\\"components-base-control components-toggle-control block-editor-responsive-block-control__toggle css-wdf2ti-Wrapper e1puf3u0\\"><div class=\\"components-base-control__field css-11vcxb9-StyledField e1puf3u1\\"><span class=\\"components-form-toggle is-checked\\"><input class=\\"components-form-toggle__input\\" id=\\"inspector-toggle-control-0\\" type=\\"checkbox\\" aria-describedby=\\"inspector-toggle-control-0__help\\" checked=\\"\\"><span class=\\"components-form-toggle__track\\"></span><span class=\\"components-form-toggle__thumb\\"></span></span><label for=\\"inspector-toggle-control-0\\" class=\\"components-toggle-control__label\\">Use the same padding on all screensizes.</label></div><p id=\\"inspector-toggle-control-0__help\\" class=\\"components-base-control__help css-1wm1a55-StyledHelp e1puf3u3\\">Toggle between using the same value for all screen sizes or using a unique value per screen size.</p></div><div class=\\"block-editor-responsive-block-control__group\\"><div class=\\"components-base-control css-wdf2ti-Wrapper e1puf3u0\\"><div class=\\"components-base-control__field css-11vcxb9-StyledField e1puf3u1\\"><div class=\\"components-flex components-select-control e1cr7zh10 css-xnuudl-Flex-Root eboqfv50\\"><div class=\\"components-flex__item e1cr7zh14 css-d373l0-Item-LabelWrapper eboqfv51\\"><label for=\\"inspector-select-control-0\\" class=\\"components-input-control__label css-za86g6-Text-BaseLabel e1cr7zh13\\"><span aria-describedby=\\"rbc-desc-0\\">All</span><span class=\\"components-visually-hidden\\" id=\\"rbc-desc-0\\">Controls the padding property for All viewports.</span></label></div><div class=\\"components-input-control__container css-nj8rmx-Container e1cr7zh11\\"><select class=\\"components-select-control__input css-owzv2l-Select e12x0a390\\" id=\\"inspector-select-control-0\\"><option value=\\"\\">Please select</option><option value=\\"small\\">Small</option><option value=\\"medium\\">Medium</option><option value=\\"large\\">Large</option></select><span class=\\"components-input-control__suffix css-1lym30p-Suffix e1cr7zh17\\"><div class=\\"css-s55r1c-DownArrowWrapper e12x0a391\\"><svg viewBox=\\"0 0 24 24\\" xmlns=\\"http://www.w3.org/2000/svg\\" width=\\"18\\" height=\\"18\\" role=\\"img\\" aria-hidden=\\"true\\" focusable=\\"false\\"><path d=\\"M17.5 11.6L12 16l-5.5-4.4.9-1.2L12 14l4.5-3.6 1 1.2z\\"></path></svg></div></span><div aria-hidden=\\"true\\" class=\\"components-input-control__backdrop css-1o1vxqx-BackdropUI e1cr7zh15\\"></div></div></div></div></div><p id=\\"all\\">All is used here for testing purposes to ensure we have access to details about the device.</p></div></div></fieldset>"`; | |||
There was a problem hiding this comment.
The diff here is like:
css-hwcz8s-Select->css-owzv2l-Selectcss-1s1n1to-BackdropUI->css-1o1vxqx-BackdropUI
Presumably this is because some of the files involved in calculating the styles have changed and the hash is updated. @ItsJonQ @saramarcondes does this seem alright? Is there a better way to snapshot styles like this so they're stable?
There was a problem hiding this comment.
Honestly a better question is whether this snapshot test is needed at all:
We're already testing for a bunch of assertions inside the container's markup, it's not clear to me, as someone who's only just looking at these tests, why the snapshot is valuable (especially considering we're using render rather than mount which makes the snapshot nearly impossible to sanely diff).
On the other hand, you didn't change anything about the styles, I have no idea why the class names that emotion generates wouldn't be idempotent based on the styles generated (not the module generating them). I wouldn't expect them to change if the results being passed into them didn't change but maybe @ItsJonQ will correct me about some part of emotion's infrastructure of which I am unaware.
Comment on lines
+11
to
+21
| /** | ||
| * @template {keyof JSX.IntrinsicElements | import('react').JSXElementConstructor<any>} T | ||
| * @typedef OwnProps | ||
| * @property {T} [as='div'] Component to render, e.g. `"div"` or `MyComponent`. | ||
| */ | ||
|
|
||
| /** | ||
| * @template {keyof JSX.IntrinsicElements | import('react').JSXElementConstructor<any>} T | ||
| * @typedef {OwnProps<T> & import('react').ComponentProps<T>} Props | ||
| */ | ||
|
|
There was a problem hiding this comment.
Nice! I was taking a look at this last night and having a hard time wrapping my head around how to type this. Good work!
|
I've rebased and fixed the conflicts, this can be reviewed. |
sarayourfriend
approved these changes
Oct 22, 2020
| * @param {any} value The incoming value. | ||
| * @param {unknown} value The incoming value. |
There was a problem hiding this comment.
Out of curiosity, why this change? I'm not fully up on when to use unknown. I guess it makes semantic sense here but what's the exact reason?
There was a problem hiding this comment.
Glad you asked 🙂
any and unknown are both boxes that we can put anything into. The difference is that any satisfies every other type, whereas unknown is the opposite. It satisfies no other type and requires us to be careful to use it. In code that's like:
// Anything _satisfies_ both types `any` and `unknown`
var __a: any = null
var __a: any = undefined
var __a: any = ['foo']
var __a: any = 0
const a: any = new Set(['apples', 'bananas'])
var __u: unknown = null
var __u: unknown = undefined
var __u: unknown = ['foo']
var __u: unknown = 0
const u: unknown = new Set(['apples', 'bananas'])
// Type system catches this. `unknown` doesn't satisfy anything! Phew…
u.toLocaleLowerCase(); // TYPE ERROR!
// We can't call set methods either, we said we didn't know anything >:/
u.has('foo'); // TYPE ERROR!
// Typically we'll use type guards to be safe with our `unknown` types :D
u instanceof Set && u.has('apples'); // No type error 👍
// `any` type satisfies everything. It's dangerous, as if we turn off the type system completely!!
a.toLocaleLowerCase() // Oops… Set doesn't implement that! We hit s runtime error "TypeError: u.toLocaleLowerCase is not a function" 💥
This was referenced Oct 22, 2020
Closed
sarayourfriend
approved these changes
Oct 23, 2020
|
Haiii! I tested out a couple of the utils. A lot of this TypeScript is a bit advanced for me 😅 . |
|
Following up with this one! It looks like the |
sirreal
force-pushed
the
update/type-more-components
branch
from
69f950e to
29930cc
Compare
|
Thanks for the feedback, everyone. Rebased to fix conflicts. |
sarayourfriend
approved these changes
Oct 29, 2020
sirreal
force-pushed
the
update/type-more-components
branch
from
29930cc to
279497d
Compare
|
Rebased, I think tests are back on track today and we can get this landed 🤞 |
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.
Description
utilsBaseControlVisuallyHiddenBuilds on #26066
Part of #18838
How has this been tested?
Type generation completes successfully.
Types of changes
Internal / Code Quality: Add Type Checking to components.
Checklist: