Adjust sorting of WordPress versions dropdown #1066
Conversation
bcotrim
force-pushed
the
add/adjust_sorting_wp_versions
branch
from
463c9da to
e9aa9a2
Compare
| }, | ||
| ...otherVersions, | ||
| ]; | ||
| let foundLatest = false; |
There was a problem hiding this comment.
| let foundLatest = false; | |
| let foundLatestStable = false; |
Nit, but this would clarify what we're looking for.
src/components/onboarding.tsx
Outdated
| const wpVersions = useRootSelector( | ||
| wordpressVersionsSelectors.selectWordPressVersionsWithLatest | ||
| ); | ||
|
|
||
| useEffect( () => { | ||
| const latestVersion = wpVersions.find( ( version ) => ! version.isBeta ); | ||
| if ( latestVersion ) { | ||
| setWpVersion( latestVersion.value ); | ||
| } | ||
| }, [ wpVersions, setWpVersion ] ); |
There was a problem hiding this comment.
Given that we're repeating this logic, I would consider adding a selectLatestStableWordPressVersion selector to src/stores/wordpress-versions-slice.ts.
There was a problem hiding this comment.
Nice, I agree. If we are repeating some code, let's make sure to unify it
| if ( ! wordpressVersionOptions.some( ( version ) => version.value === currentWpVersion ) ) { | ||
| wordpressVersionOptions.push( { label: currentWpVersion, value: currentWpVersion } ); | ||
| const customVersion = { label: currentWpVersion, value: currentWpVersion }; |
There was a problem hiding this comment.
Total nit, but I would move this below the findIndex call
|
In terms of functionality, everything looks good for me ❤️ I am taking a look at the code now |
| } | ||
| return semver.lt( compareVer, currentVer ); | ||
| } ); | ||
| // if the current version is less than all the versions in the list, add it to the end |
There was a problem hiding this comment.
We can probably clean those up. Although, not a strong preference considering that there are a lof of things happening
There was a problem hiding this comment.
+1
Together with removing this // otherwise, insert it at the found index.
And I think that my suggestion to rename insertIndex to indexOfPrevVersion will make removing comments more reasonable.
| // find the index of the version that is less than the current version | ||
| const insertIndex = wordpressVersionOptions.findIndex( ( compareVersion ) => { |
There was a problem hiding this comment.
What about this, to avoid comment and make variable more obvious to "what it is", i stead of "what it should do":
| // find the index of the version that is less than the current version | |
| const insertIndex = wordpressVersionOptions.findIndex( ( compareVersion ) => { | |
| const indexOfPrevVersion = wordpressVersionOptions.findIndex( ( compareVersion ) => { |
There was a problem hiding this comment.
Also, let's extract this insertion to a separate function. Ar a result the component will be smaller and easy to read.
if ( ! wordpressVersionOptions.some( ( version ) => version.value === currentWpVersion ) ) {
addWpVersionToList( currentWpVersion, wordpressVersionOptions );
}
| } | ||
| return semver.lt( compareVer, currentVer ); | ||
| } ); | ||
| // if the current version is less than all the versions in the list, add it to the end |
There was a problem hiding this comment.
+1
Together with removing this // otherwise, insert it at the found index.
And I think that my suggestion to rename insertIndex to indexOfPrevVersion will make removing comments more reasonable.
… stable wp release
|
Thank you all for the feedback, while moving the function to lib folder and doing the tests, I noticed we weren't handling the case for a version newer than all the versions. Probably an edge case, but I think we shouldn't assume that. |
Related issues
Proposed Changes
lateston topsemver.coerce()to properly compare versions that aren't in the official version listTesting Instructions
Additional Testing for Custom Versions:
Pre-merge Checklist