Navigator: add NavigatorButton and NavigatorBackButton components
#38634
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: +53 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
ciampo
force-pushed
the
feat/components-navigator-link
branch
from
5230935 to
596eb0d
Compare
ciampo
added
[Feature] Component System
mirka
reviewed
Feb 9, 2022
packages/components/src/navigator/navigator-back-link/component.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/navigator/navigator-back-link/component.tsx
Outdated
Show resolved
Hide resolved
| * <NavigatorProvider initialPath="/"> | ||
| * <NavigatorScreen path="/"> | ||
| * <p>This is the home screen.</p> | ||
| * <NavigatorLink variant="secondary" path="/child"> |
There was a problem hiding this comment.
I think we should avoid having this variant="secondary" prop in all our examples, because it's a bit confusing on first glance and leaks the fact that it's internally a Button.
I see a few possibilities here:
- If the intent is to give it
secondarystyling by default, theNavigatorLinkcomponent should perhaps handle that internally. - If we do want to highlight that it's internally a
Button, that should probably be made clearer in the docs. - Looking at the actual use cases in our codebase though, I think we can assume that most consumers will be using this via
asprop or hook. Should we maybe highlight that kind of example instead? If this is the main use case, we could even simply further and make the internal component a lowercase-b<button>.
Probably some combination of 1 and 3?
There was a problem hiding this comment.
If the intent is to give it secondary styling by default, the NavigatorLink component should perhaps handle that internally.
This was partially the intent. But your point makes sense — I went ahead and removed variant="secondary" from the examples in 68fac76.
Also wanted to note that we're using that same prop in the Storybook examples
Looking at the actual use cases in our codebase though, I think we can assume that most consumers will be using this via as prop or hook. Should we maybe highlight that kind of example instead? If this is the main use case, we could even simply further and make the internal component a lowercase-b .
Not not about a lot of consumers using the as prop — I think that using Button is a sensible default that may cover a good amount of scenarios. For more context, this was discussed originally in #37223 (comment)
If we do want to highlight that it's internally a Button, that should probably be made clearer in the docs.
Related to #37223 (comment), actually there's an interesting point that caught my attention:
I believe that Button, when passed an href prop, automatically renders as a <a /> instead of a <button />
In the light of the above, should we disallow href as a prop from NavigatorLink? I could then go ahead and add an additional paragraph at the end of the Props section, similarly to https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/card/card/README.md?plain=1#L67-L69
There was a problem hiding this comment.
I believe that
Button, when passed anhrefprop, automatically renders as a<a />instead of a<button />In the light of the above, should we disallow
hrefas a prop fromNavigatorLink? I could then go ahead and add an additional paragraph at the end of thePropssection, similarly to https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/card/card/README.md?plain=1#L67-L69
Ah, that's right. Let's do that.
packages/components/src/navigator/navigator-back-link/README.md
Outdated
Show resolved
Hide resolved
packages/components/src/navigator/navigator-back-link/README.md
Outdated
Show resolved
Hide resolved
ciampo
force-pushed
the
feat/components-navigator-link
branch
from
b284a15 to
4377794
Compare
mirka
approved these changes
Feb 14, 2022
Comment on lines
4
to
5
| // eslint-disable-next-line no-restricted-imports | ||
| import type { Ref } from 'react'; |
There was a problem hiding this comment.
One more here 🧹
Suggested change
| // eslint-disable-next-line no-restricted-imports | |
| import type { Ref } from 'react'; | |
| import type { ForwardedRef } from 'react'; |
There was a problem hiding this comment.
Woops! Done in 47f4b1f
Comment on lines
+382
to
+383
| it( 'should escape the value of the `path` prop', () => { | ||
| render( <MyNavigation /> ); |
There was a problem hiding this comment.
Thank you! Once the main "testing env" is set up, it actually becomes quite a trivial task
There was a problem hiding this comment.
Any reason for the path value escaping?
There was a problem hiding this comment.
Since the path prop is used as the value for an HTML attribute (part of the focus restoration logic), the value of path needs to be a valid value for HTML attributes (meaning it can't contain a series of characters like single/double quotes, spaces, etc).
Therefore, the value of path is escaped to make sure that it can be safely applied as an HTML attribute — and this test checks that behaviour
…gatorBackButton`
ciampo
force-pushed
the
feat/components-navigator-link
branch
from
7f1b257 to
5c584a5
Compare
|
Addressed the last round of feedback and rebased on top of Unless there's any other feedback, I'll merge if all checks pass! |
ciampo
force-pushed
the
feat/components-navigator-link
branch
from
88de38a to
bfb596d
Compare
ciampo
force-pushed
the
feat/components-navigator-link
branch
from
bfb596d to
e7866cb
Compare
ciampo
changed the title
Navigator: add NavigatorLink and NavigatorBackLink componentsNavigator: add NavigatorButton and NavigatorBackButton components
8 tasks
cbravobernal
changed the title
Navigator: add NavigatorButton and NavigatorBackButton componentsNavigatorButton and NavigatorBackButton components
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
Following the conversation in #37063 and the exploratory work done in #37223, this PR is the first step towards adding support for advanced focus restoration in
Navigator. The plan is to have the work split across 3 PRs:Navigator: add basic location history #37416)Navigator: add focus restoration #38149)NavigationButtonandNavigationBackButtoncomponents (this PR)This PR contains the following changes:
NavigatorButtonandNavigatorBackButtonNavigatorfamilyNavigatorcomponent (in the post editor's preferences modal, and in the Global Styles sidebar)Note: compared to before, the HTML attribute used for the focus restoration is
id(as opposed to a customdataattribute). This can be changed through theattributeNameprop on theNavigatorLinkcomponent, in case.Testing Instructions
No visual or behavioral changes are expected — this refactor should just make it easier to work with the
Navigatorcomponents:Screenshots
Types of changes
New feature (introduction of two new components)
Checklist:
*.native.jsfiles for terms that need renaming or removal).