Move getStablePath function into the wordrpess/url package #35992
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
…dpress/url package.
…dpress/url package (because we've moved normalizePath to the wordpress/url package).
anton-vlasenko
added
[Package] Url
anton-vlasenko
requested review from
adamziel,
draganescu,
getdave,
kevin940726,
mmtr,
nerrad,
talldan and
tellthemachines
as code owners
|
Size Change: +3.67 kB (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
anton-vlasenko
changed the title
anton-vlasenko
changed the title
kevin940726
approved these changes
Oct 28, 2021
Comment on lines
+18
to
+33
| // 'b=1&c=2&a=5' | ||
| return ( | ||
| base + | ||
| '?' + | ||
| query | ||
| // [ 'b=1', 'c=2', 'a=5' ] | ||
| .split( '&' ) | ||
| // [ [ 'b, '1' ], [ 'c', '2' ], [ 'a', '5' ] ] | ||
| .map( ( entry ) => entry.split( '=' ) ) | ||
| // [ [ 'a', '5' ], [ 'b, '1' ], [ 'c', '2' ] ] | ||
| .sort( ( a, b ) => a[ 0 ].localeCompare( b[ 0 ] ) ) | ||
| // [ 'a=5', 'b=1', 'c=2' ] | ||
| .map( ( pair ) => pair.join( '=' ) ) | ||
| // 'a=5&b=1&c=2' | ||
| .join( '&' ) | ||
| ); |
There was a problem hiding this comment.
Now that we don't have to support IE, we can do this instead:
const searchParams = new URLSearchParams( query );
searchParams.sort();
return `${base}?${ searchParams.toString() }`;There was a problem hiding this comment.
That's a good idea. Thank you, @kevin940726 !
I've updated the changelog files in d724344
UPD:
Left a potential improvement we could do, but not necessary for this PR.
I've created an issue for this improvement: #36046
kevin940726
reviewed
Oct 29, 2021
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ### Internal | |||
There was a problem hiding this comment.
I don't think we need to add changelog for internal function. They aren't expected to be used by users anyway.
There was a problem hiding this comment.
@kevin940726
Excuse me, but I don't understand why we don't have to add such changes to the changelog.
This document states that:
For each pull request, you should always include relevant changes in a "Unreleased" heading at the top of the file. You should add the heading if it doesn't already exist.
Also, it says this:
"Internal" - Changes which do not have an impact on the public interface or behavior of the module (requires a patch version bump).
There was a problem hiding this comment.
Huh, I guess that we're just not strictly following this 😅 ? Anyway, this looks fine to me.
There was a problem hiding this comment.
I guess that we're just not strictly following this
Yes, it looks like so. 😅
But maybe we should (I am not excluding myself from the list).
Again, thank you for the review!
andrewserong
added
the
[Type] Code Quality
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
We have some duplicate code in both
wordpress/api-fetchandwordpress/edit-navigationpackages.Both of these packages have the
getStablePathfunction defined.This PR aims to move
getStablePathinto thewordpress/urlpackage and rename it tonormalizePath. Thus, it can be used by bothwordpress/api-fetchandwordpress/edit-navigationpackages.Fixes #35799.
How has this been tested?
This PR aims to refactor the
getStablePathfunction. So, to test this PR, it's enough to verify that there are nonormalizePath/getStablePathrelated errors in the console when you open Navigation Editor.Screenshots
No screenshots are needed.
Types of changes
New feature (refactoring)
Checklist:
*.native.jsfiles for terms that need renaming or removal).