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.

Sign up for GitHub

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

Jump to bottom

Edit Navigation: Replace remaining store name string with constant #27281

Merged
merged 7 commits into from
Nov 26, 2020
Merged

Edit Navigation: Replace remaining store name string with constant #27281

merged 7 commits into from
Nov 26, 2020

Conversation

rafaelgallani
Copy link
Contributor

Description

Addresses #27182 and #27088. Replaces the remaining references to the core/edit-navigation store name (hardcoded string) with the exported value from the constants file. I also exported the constant value in the edit-navigation package so that other packages can use it when necessary. If that is not the best way to do it, please let me know. 😸

How has this been tested?

npm run test, no additional test was done.

Screenshots

edit_navigation_2

Types of changes

Code refactoring, non-breaking change.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@rafaelgallani
Copy link
Contributor Author

Well, I tried using the same branch. Apparently, all the commits from the previous PR are also being shown in the commit section, even though only one of them is new, compared to master. Sorry about it. I can create another branch if that's necessary. 😸

@rafaelgallani rafaelgallani force-pushed the refactor/replace-edit-nav-store-key-with-store-def branch from ec28181 to ff73e73 Compare November 25, 2020 22:11
@rafaelgallani
Copy link
Contributor Author

The E2E tests have failed again, same as #27178: I made an empty commit to trigger the pipelines, and it ran without any errors.

@gziolo gziolo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Edit Widgets /packages/edit-widgets labels Nov 26, 2020
gziolo
gziolo reviewed Nov 26, 2020
View reviewed changes
packages/edit-widgets/src/store/controls.js Outdated
@@ -74,7 +75,7 @@ export function getWidgetToClientIdMapping() {
export function getNavigationPostForMenu( menuId ) {
return {
type: 'SELECT',
registryName: 'core/edit-navigation',
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave it as a hardcoded string and revert the change in this line. We shouldn't expose new constants from @wordpress/edit-navigation. It is probably something complex that we better ignore in this PR.

@adamziel and @noisysocks, aren't edit-widgets and edit-navigation two packages that target two different pages? I'm curious how it works since @wordpress/edit-widgets doesn't depend on @wordpress/edit-navigation as you can learn from the package.json file changes proposed in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

It's probably just a mistake. edit-widgets's store is originally copied from edit-navigation, seems like we just forgot to delete this.

gziolo
gziolo requested changes Nov 26, 2020
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Let's remove the bits that expose STORE_NAME for @wordpress/edit-widgets package and this PR will be good to go. Thank you for looking into it.

@gziolo gziolo added [Package] Edit Navigation and removed [Package] Edit Widgets /packages/edit-widgets labels Nov 26, 2020
@gziolo gziolo mentioned this pull request Nov 26, 2020
Closed
17 tasks
@rafaelgallani
Copy link
Contributor Author

OK, sorry about the edit-widgets package and the exposed string in the public API.
Since these are my first contributions to the project, I didn't know how to handle it well. I'll make the changes in a moment 😅

kevin940726
kevin940726 approved these changes Nov 26, 2020
View reviewed changes
Copy link
Member

@kevin940726 kevin940726 left a comment

Choose a reason for hiding this comment

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

LGTM :) Awesome job! 💯

@gziolo gziolo merged commit 2fd7a1c into WordPress:master Nov 26, 2020
@github-actions github-actions bot added this to the Gutenberg 9.5 milestone Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevin940726 kevin940726 kevin940726 approved these changes

@gziolo gziolo gziolo approved these changes

@adamziel adamziel Awaiting requested review from adamziel

@draganescu draganescu Awaiting requested review from draganescu

@noisysocks noisysocks Awaiting requested review from noisysocks

@talldan talldan Awaiting requested review from talldan

@tellthemachines tellthemachines Awaiting requested review from tellthemachines

Assignees
No one assigned
Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 9.5
Development

Successfully merging this pull request may close these issues.
3 participants
@rafaelgallani @gziolo @kevin940726