NavigableContainer: convert to TypeScript
#49377
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
kienstra
commented
Apr 1, 2023
kienstra
commented
Apr 1, 2023
kienstra
commented
Apr 1, 2023
kienstra
commented
Apr 1, 2023
|
Size Change: +41 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
kienstra
commented
Apr 1, 2023
kienstra
commented
Apr 1, 2023
packages/components/src/navigable-container/stories/navigable-menu.tsx
Outdated
Show resolved
Hide resolved
kienstra
commented
Apr 1, 2023
packages/components/src/navigable-container/test/navigable-menu.tsx
Outdated
Show resolved
Hide resolved
|
Flaky tests detected in eb8b909. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4710317571
|
kienstra
commented
Apr 1, 2023
packages/components/src/navigable-container/test/tababble-container.tsx
Outdated
Show resolved
Hide resolved
kienstra
commented
Apr 1, 2023
kienstra
commented
Apr 1, 2023
kienstra
commented
Apr 1, 2023
kienstra
commented
Apr 1, 2023
kienstra
commented
Apr 1, 2023
|
Hi @ciampo, |
ciampo
reviewed
Apr 5, 2023
| const { onlyBrowserTabstops } = this.props; | ||
| const finder = onlyBrowserTabstops ? focus.tabbable : focus.focusable; | ||
| const focusables = finder.find( this.container ); | ||
| const focusables = finder.find( this.container ) as HTMLElement[]; |
There was a problem hiding this comment.
finder.find returns an Element[]. Not something for this PR, but it would be interesting to understand if the return type could be safely narrowed to HTMLElement[]
packages/components/src/navigable-container/stories/navigable-menu.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/navigable-container/test/tababble-container.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/navigable-container/test/tababble-container.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/navigable-container/test/navigable-menu.tsx
Outdated
Show resolved
Hide resolved
|
@ciampo, |
ciampo
reviewed
Apr 15, 2023
There was a problem hiding this comment.
We still need to tweak the Storybook settings:
- probably disable
childrencontrols - remove other unnecessary controls, since the same info can be inferred from the TypeScript types
- add
parametersto enable controls and docs
Proposed diff
diff --git a/packages/components/src/navigable-container/stories/navigable-menu.tsx b/packages/components/src/navigable-container/stories/navigable-menu.tsx
index 0c433e01fd..5f8ee2e4e5 100644
--- a/packages/components/src/navigable-container/stories/navigable-menu.tsx
+++ b/packages/components/src/navigable-container/stories/navigable-menu.tsx
@@ -13,11 +13,13 @@ const meta: ComponentMeta< typeof NavigableMenu > = {
component: NavigableMenu,
argTypes: {
children: { control: { type: null } },
- onNavigate: { action: 'onNavigate' },
- orientation: {
- options: [ 'horizontal', 'vertical' ],
- control: { type: 'radio' },
+ },
+ parameters: {
+ actions: { argTypesRegex: '^on.*' },
+ controls: {
+ expanded: true,
},
+ docs: { source: { state: 'open' } },
},
};
export default meta;
diff --git a/packages/components/src/navigable-container/stories/tabbable-container.tsx b/packages/components/src/navigable-container/stories/tabbable-container.tsx
index 8b9221ea54..b517019e29 100644
--- a/packages/components/src/navigable-container/stories/tabbable-container.tsx
+++ b/packages/components/src/navigable-container/stories/tabbable-container.tsx
@@ -12,11 +12,14 @@ const meta: ComponentMeta< typeof TabbableContainer > = {
title: 'Components/TabbableContainer',
component: TabbableContainer,
argTypes: {
- children: { type: undefined },
- cycle: {
- type: 'boolean',
+ children: { control: { type: null } },
+ },
+ parameters: {
+ actions: { argTypesRegex: '^on.*' },
+ controls: {
+ expanded: true,
},
- onNavigate: { action: 'onNavigate' },
+ docs: { source: { state: 'open' } },
},
};
export default meta;
ciampo
reviewed
Apr 15, 2023
ciampo
reviewed
Apr 15, 2023
kienstra
added a commit
that referenced
this pull request
Apr 15, 2023
…ontainer/container.tsx Co-authored-by: Marco Ciampini <[email protected]>
…ontainer/stories/navigable-menu.tsx Co-authored-by: Marco Ciampini <[email protected]>
…ontainer/container.tsx Co-authored-by: Marco Ciampini <[email protected]>
…ontainer/container.tsx Co-authored-by: Marco Ciampini <[email protected]>
ciampo
force-pushed
the
update/navigable-container-ts
branch
from
7dc3c71
to
a8b6bf5
Compare
|
cc @chad1008 |
|
Oh all good! Thanks for your great reviews as always! |
bph
added
[Type] Code Quality
23 tasks
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?
Convert
NavigableContainercomponents to TypeSccriptWhy?
As part of #35744
How?
Mainly by adding types
Testing Instructions
npm run storybook:devNavigableMenuandTabbableContainerTesting Instructions for Keyboard
npm run storybook:devNavigableMenuandTabbableContainerScreenshots or screencast
See above