Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Navigator: stabilize and export APIs #64613

Merged
merged 38 commits into from
Sep 27, 2024

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Aug 19, 2024

What?

Part of #59418
Supersedes #60927
Follow-up to #63317

Export the Navigator component, its subcomponents, and the useNavigator hook as stable APIs from the @wordpress/components package

Why?

This is part of a larger effort to remove __experimental prefix from all "experimental" components, effectively promoting them to regular stable components. See the related issue for more context.

How?

  • Aside from the existing legacy exports, which use the __experimental prefix, add new, unprefixed exports:
    • Legacy exports stay untouched;
    • The new, stable exports will adopt slightly different names compared to legacy and the overloaded naming convention that we agreed to apply for component components:
      • NavigatorProvider => Navigator
      • NavigatorScreen => Navigator.Screen
      • NavigatorButton => Navigator.Button
      • NavigatorBackButton => Navigator.BackButton
      • NavigatorToParentButton won't be exported since it's already deprecated in the experimental version of the component
  • Updated all JSDocs and README
  • Updated Storybook and Unit Tests to use the stable version of the component
  • Added Storybook redirect to avoid getting broken links
  • A few smaller changes:
    • Updated the internal context namespacing (which shouldn't be a breaking change since context is an internal API of the @wordpress/components package)

Next steps?

  • refactor existing usages in Gutenberg to stable version
  • deprecate experimental version (both in JSDocs and with a runtime warning) (+ add unit tests for deprecation warnings)

Testing Instructions

  • Technically there shouldn't be any runtime (behavioral) changes to Navigator;
  • Make sure that Storybook examples are documented correctly and work as expected;
  • Smoke test Navigator usages across the editor;
  • Review JSDocs, READMEs, and IntelliSense snippets for both legacy and stable APIs

✍️  Dev note

The legacy set of __experimentalNavigator* APIs is deprecated and should instead be imported as Navigator. All of the sub-components are also available via the Navigator namespace.

Moreover, the __experimentalNavigatorToParentButton component and the goToParent method available via the __experimentalUseNavigator hook are now deprecated, and they now behave identically to the __experimentalNavigatorBackButton and the goBack method.

To recap:

  • __experimentalNavigatorProvider => Navigator
  • __experimentalNavigatorScreen => Navigator.Screen
  • __experimentalNavigatorButton => Navigator.Button
  • __experimentalNavigatorBackButton => Navigator.BackButton
  • __experimentalNavigatorToParentButton => Navigator.BackButton
  • __experimentalUseNavigator => useNavigator

Copy link
Contributor Author

@ciampo ciampo Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Legacy exports have been moved to a separate legacy.ts file (including the legacy JSDocs mentioning the components with their experimental export name).

This makes it easy to have separate names from the stable version, different JSDocs, and deprecation warnings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New exports (and their up-to-date JSDocs) have been moved to the folder's main index.ts file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All READMEs have also been updated:

  • removed experimental snippet
  • updated the naming of all Navigator sub-components, including internal links to other READMEs

Copy link
Contributor Author

@ciampo ciampo Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was actually renamed to packages/components/src/navigator/navigator/index.tsx, but GitHub is not picking that up

* The `NavigatorProvider` component allows rendering nested views/panels/menus
* (via the `NavigatorScreen` component and navigate between these different
* view (via the `NavigatorButton` and `NavigatorBackButton` components or the
* The `Navigator` component allows rendering nested views/panels/menus
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The top level Navigator component (previously named NavigatorProvider) is the only component for which I've kept a JSDoc for the internal implementation, because apparently Storybook uses it when displaying docs 🤷 Not sure why it doesn't use the one defined in packages/components/src/navigator/index.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like the ts extension was the problem — changing it to .tsx seems to allow Storybook to pick the JSDoc up 😅 85471a9

cc @mirka

// @ts-expect-error - See https://github.com/storybookjs/storybook/issues/23170
BackButton: Navigator.BackButton,
},
title: 'Components/Navigator',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PREVIOUSLY_EXPERIMENTAL_COMPONENTS in storybook/manager-head.html to make sure old links still work

@ciampo ciampo self-assigned this Aug 19, 2024
@ciampo ciampo added [Package] Components /packages/components [Type] New API New API to be used by plugin developers or package users. labels Aug 19, 2024
@ciampo ciampo marked this pull request as ready for review August 19, 2024 14:51
@ciampo ciampo requested review from youknowriad and a team August 19, 2024 14:51
Copy link

github-actions bot commented Aug 19, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ciampo ciampo added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Aug 19, 2024
@ciampo ciampo force-pushed the feat/stabilize-navigator/introduce-stable-version branch from 965220a to 85471a9 Compare August 21, 2024 15:37
Comment on lines -1106 to -1117
{
"title": "NavigatorProvider",
"slug": "navigator-provider",
"markdown_source": "../packages/components/src/navigator/navigator-provider/README.md",
"parent": "components"
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, removing things from the manifest is not going to remove it from the Block Editor Handbook 😱 There are some extra steps, see #60003 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I'll follow-up accordingly

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a reminder we'll need to take care of this one after merging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @WordPress/wordpress-org-meta-team , could you help with this? We need to update the Block Editor Handbook, removing the following components:

If possible, we'd also like to set a redirect for those pages being removed. The redirect should point to the Navigator component.

Could you please help with that? Thank you 🙏

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These updates have shipped. I noticed the formatting on the new doc loses consistency around the Navigator.Button point (heading is not bold):

Screenshot 2024-10-01 at 10 42 06 AM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

I noticed the formatting on the new doc loses consistency around the Navigator.Button point (heading is not bold):

Good catch! Opened #65763 to fix it

packages/components/CHANGELOG.md Outdated Show resolved Hide resolved
packages/components/src/navigator/index.tsx Outdated Show resolved Hide resolved
packages/components/src/navigator/index.tsx Show resolved Hide resolved
packages/components/src/navigator/index.tsx Outdated Show resolved Hide resolved
packages/components/src/navigator/test/index.tsx Outdated Show resolved Hide resolved
@ciampo
Copy link
Contributor Author

ciampo commented Aug 22, 2024

I'm going to resume work on this PR after #64675 is merged.

@tyxla
Copy link
Member

tyxla commented Aug 23, 2024

I'm going to resume work on this PR after #64675 is merged.

Sounds good. I was starting to review it, but I will halt it for now.

I wonder if it's a good idea to approach this modularly and split whatever we can to separate PRs to keep this one smaller.

@ciampo ciampo force-pushed the feat/stabilize-navigator/introduce-stable-version branch from 85471a9 to 0f2d077 Compare August 26, 2024 09:03
@ciampo
Copy link
Contributor Author

ciampo commented Aug 26, 2024

I rebased and addressed simple feedback.

I would still wait before reviewing this PR. The current plan of action is:

I've also updated the tracking issue adding all of the planned follow-up step to this PR.

I wonder if it's a good idea to approach this modularly and split whatever we can to separate PRs to keep this one smaller.

@tyxla , Even if the size of the PR is medium-large, I believe 99% of the changes left in this PR are simple renames — and in that sense I feel ok with the current size. Having said that, I'll re-assess once all dependency PRs are merged, before asking for a final round of review.

@tyxla
Copy link
Member

tyxla commented Aug 28, 2024

@tyxla , Even if the size of the PR is medium-large, I believe 99% of the changes left in this PR are simple renames — and in that sense I feel ok with the current size. Having said that, I'll re-assess once all dependency PRs are merged, before asking for a final round of review.

That makes sense, @ciampo, will help with the review once all the dependencies are merged 👍

@ciampo ciampo force-pushed the feat/stabilize-navigator/introduce-stable-version branch from cea2b8e to 999795a Compare September 27, 2024 14:25
@ciampo ciampo enabled auto-merge (squash) September 27, 2024 14:31
@ciampo ciampo merged commit 6f0fd8e into trunk Sep 27, 2024
64 checks passed
@ciampo ciampo deleted the feat/stabilize-navigator/introduce-stable-version branch September 27, 2024 15:07
@github-actions github-actions bot added this to the Gutenberg 19.4 milestone Sep 27, 2024
adamwoodnz added a commit to WordPress/wporg-developer that referenced this pull request Sep 30, 2024
@fabiankaegy fabiankaegy mentioned this pull request Oct 1, 2024
97 tasks
huubl pushed a commit to huubl/gutenberg that referenced this pull request Oct 2, 2024
* Remove experimental status from Storybook

* NavigatorProvider => Navigator (internally)

* Move legacy exports to separate file

* Export overloaded notation

* Use overloaded notation in Storybook

* Use overloaded notation in unit tests (skipping deprecated component for now)

* Update docs manifest.json

* Navigator: update docs

* Navigator.BackButton: update docs

* NavigatorToParentButton: delete README

* Navigator.Button: update docs

* NavigatorScreen: update docs

* Restore `NavigatorToParentButton` test

* Use dot notation in context namespaces too

* Use named export for `NavigatorToParentButton` too

* Fixup docs manifest

* Export stable version from components package

* CHANGELOG

* Change index.ts extension to tsx to allow Storybook to extract JSDocs better, remove duplicate JSDocs

* Remove unnecessary and mispelled display name for top-level `Navigator`

* Remove @example tag from top-level export

* Clean up unused imports

* Fix storybook styles

* Update new section of the docs

* Update more docs

* Update deprecation warning

* Update and align docs

* Fix tests warning checks

* Typo

* Fix useContextSystem names

* Remove mentions of the `useNavigator` hook

* useNavigator JSDocs

* Move all READMEs under the main README

* Move README to component's root folder

* Update types JSDocs to match README

* Update docs manifest

* Fix JSDocs typos, grammar, and broken links.

* Update storybook selector

---

Co-authored-by: ciampo <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>
huubl added a commit to huubl/gutenberg that referenced this pull request Oct 2, 2024
@ciampo ciampo added has dev note when dev note is done (for upcoming WordPress release) and removed Needs Dev Note Requires a developer note for a major WordPress release cycle labels Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has dev note when dev note is done (for upcoming WordPress release) [Package] Components /packages/components [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants