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

Using addQueryArgs to generate Manage All Reusable Blocks link #10065

Closed

Conversation

SofiaSousa
Copy link
Contributor

@SofiaSousa SofiaSousa commented Sep 20, 2018

Description

Using addQueryArgs method from @wordpress/url instead of hand coding 'Manage All Reusable Blocks' link.

How has this been tested?

  1. Open 'Reusable' panel in 'Add Block' menu
  2. Check 'Manage All Reusable Blocks' link is redirecting to edit.php?post_type=wp_block.
  3. Open 'More' menu (top right ...)
  4. Check 'Manage All Reusable Blocks' link is redirecting to edit.php?post_type=wp_block.

Screenshots

Types of changes

Reuse an existing method.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@SofiaSousa SofiaSousa changed the title feat: Use addQueryArgs to generate Manage All Reusable Blocks link Using addQueryArgs to generate Manage All Reusable Blocks link Sep 20, 2018
@chrisvanpatten chrisvanpatten added [Type] Enhancement A suggestion for improvement. [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Type] Code Quality Issues or PRs that relate to code quality labels Sep 20, 2018
@youknowriad
Copy link
Contributor

What's the benefit for using addQueryArgs if the URL is fixed?

@SofiaSousa
Copy link
Contributor Author

To be coherent. Then in our case, we override addQueryArgs in order to apply the changes we need on the editor paths since paths are not configurable.

@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Jan 31, 2019
@gziolo
Copy link
Member

gziolo commented Jan 31, 2019

I think it's fine to land it. @SofiaSousa can you refresh this PR with the latest changes from master?

In other places, I also see getWPAdminURL function uses. Did you consider using this one instead?

@SofiaSousa
Copy link
Contributor Author

@gziolo PR has the latest changes from the master now.

I did a little search for getWPAdminURL and figured out that is just an "abstraction" of addQueryArgs. I don't know the reason for this, but probably there is a good one :)

Otherwise, I think we should replace all calls of getWPAdminURL.

@gziolo
Copy link
Member

gziolo commented Feb 1, 2019

I did a little search for getWPAdminURL and figured out that is just an "abstraction" of addQueryArgs. I don't know the reason for this, but probably there is a good one :)

I'm confused as much as you are, good finding :)

@youknowriad a553056

  • can we deprecate getWPAdminURL? I would prefer if we had a way to abstract away all links and make them filterable if plugins want to change those links or being easy to override if projects like G in Drupal prefers to do so.

I think addQueryArgs is fine for now in that case.

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.

Other than mentioned lock file changes, these changes look good. Thanks for refreshing.

@@ -2975,7 +2975,6 @@
"resolved": "https://registry.npmjs.org/align-text/-/align-text-0.1.4.tgz",
"integrity": "sha1-DNkKVhCT810KmSVsIrcGlDP60Rc=",
"dev": true,
"optional": true,
Copy link
Member

Choose a reason for hiding this comment

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

I see unrelated lock file changes in this PR. My bet is that it is because this file was regenerated using npm 6.7 which introduced some changes in a way optional flag is handled. Can you try to run npm install with the latest version of npm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which version did you mean? I had npm@6.1.0 when those "optional": true, appeared. Tried to update to the latest (npm install -g npm@latest) but the latest version is npm@6.7.0.

Copy link
Member

Choose a reason for hiding this comment

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

the latest version is npm@6.7.0

This one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I misunderstood your comment.

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.

I opened #13653 to fix the issue with the lock file. I tested those changes and they work as expected. Thank you for opening this PR. I will land your changes from my branch, but technically it will still get merged under your name :)

@gziolo gziolo closed this Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants