Skip to content

Using addQueryArgs to generate Manage All Reusable Blocks link#10065

Closed
SofiaSousa wants to merge 8 commits intoWordPress:masterfrom
front:try/manage-reusable-blocks-link
Closed

Using addQueryArgs to generate Manage All Reusable Blocks link#10065
SofiaSousa wants to merge 8 commits intoWordPress:masterfrom
front:try/manage-reusable-blocks-link

Conversation

@SofiaSousa
Copy link
Copy Markdown
Contributor

@SofiaSousa SofiaSousa commented Sep 20, 2018 •
edited
Loading

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
gziolo
gziolo reviewed Sep 24, 2018
View reviewed changes
@youknowriad
Copy link
Copy Markdown
Contributor

youknowriad commented Sep 24, 2018

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

@SofiaSousa
Copy link
Copy Markdown
Contributor Author

SofiaSousa commented Sep 24, 2018

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
Copy Markdown
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
Copy Markdown
Contributor Author

SofiaSousa commented Jan 31, 2019

@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
Copy Markdown
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.

gziolo
gziolo requested changes Feb 1, 2019
View reviewed changes
Copy link
Copy Markdown
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.

package-lock.json
"resolved": "https://registry.npmjs.org/align-text/-/align-text-0.1.4.tgz",
"integrity": "sha1-DNkKVhCT810KmSVsIrcGlDP60Rc=",
"dev": true,
"optional": true,
Copy link
Copy Markdown
Member

@gziolo gziolo Feb 1, 2019

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
Copy Markdown
Contributor Author

@SofiaSousa SofiaSousa Feb 1, 2019

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 [email protected] when those "optional": true, appeared. Tried to update to the latest (npm install -g npm@latest) but the latest version is [email protected].

Copy link
Copy Markdown
Member

@gziolo gziolo Feb 1, 2019

Choose a reason for hiding this comment

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

the latest version is [email protected]

This one

Copy link
Copy Markdown
Contributor Author

@SofiaSousa SofiaSousa Feb 3, 2019

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.

@gziolo gziolo mentioned this pull request Feb 4, 2019
Merged
4 tasks
gziolo
gziolo approved these changes Feb 4, 2019
View reviewed changes
Copy link
Copy Markdown
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

Reviewers

@gziolo gziolo gziolo approved these changes

@youknowriad youknowriad Awaiting requested review from youknowriad

@noisysocks noisysocks Awaiting requested review from noisysocks

Assignees

No one assigned

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

Milestone

5.1 (Gutenberg)

Development

Successfully merging this pull request may close these issues.

4 participants

@SofiaSousa @youknowriad @gziolo @chrisvanpatten