-
Notifications
You must be signed in to change notification settings - Fork 29
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
Adjust sorting of WordPress versions dropdown #1066
base: trunk
Are you sure you want to change the base?
Conversation
463c9da
to
e9aa9a2
Compare
}, | ||
...otherVersions, | ||
]; | ||
let foundLatest = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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
latest
on topsemver.coerce()
to properly compare versions that aren't in the official version listTesting Instructions
Additional Testing for Custom Versions:
Pre-merge Checklist