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

Adjust sorting of WordPress versions dropdown #1066

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

bcotrim
Copy link
Contributor

@bcotrim bcotrim commented Mar 13, 2025

Related issues

Proposed Changes

  • Adjust the order or the wordpress versions, to preserve the semver order, instead of listing latest on top
  • Ensure the latest version is selected by default on add site modal
  • Improve version ordering logic when handling custom/non-listed WordPress versions
    • Use semver.coerce() to properly compare versions that aren't in the official version list
    • Ensure custom versions are inserted in the correct position based on semver comparison
Onboarding - Default Onboarding - Dropdown Add Site - Default Add Site - Dropdown Edit Site - Default Edit Site - Dropdown
image image image image image image

Testing Instructions

  • Start the onboarding flow (delete all sites)
  • Confirm the default WordPress version selected value is the latest version
  • Confirm the order from the WordPress version dropdown is correct
  • Adding a site should result in a site with the selected WordPress version
  • Try for multiple versions and without selecting a version from the dropdown to ensure the values are correctly set
  • Repeat the process for Add Site workflow
  • Check Edit site in Settings
  • Confirm the site WordPress version is the version selected in the Edit site modal
  • Confirm the order of the WordPress version dropdown

Additional Testing for Custom Versions:

  • Install a WordPress version that isn't in the default version list (e.g. 5.8)
  • Verify that the custom version appears in the correct position in the version dropdown based on semver ordering
  • Test with both older and newer versions to ensure proper ordering in all cases

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@bcotrim bcotrim force-pushed the add/adjust_sorting_wp_versions branch from 463c9da to e9aa9a2 Compare March 13, 2025 16:35
@bcotrim bcotrim requested a review from a team March 13, 2025 17:17
@bcotrim bcotrim marked this pull request as ready for review March 13, 2025 17:17
},
...otherVersions,
];
let foundLatest = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let foundLatest = false;
let foundLatestStable = false;

Nit, but this would clarify what we're looking for.

Comment on lines 90 to 99
const wpVersions = useRootSelector(
wordpressVersionsSelectors.selectWordPressVersionsWithLatest
);

useEffect( () => {
const latestVersion = wpVersions.find( ( version ) => ! version.isBeta );
if ( latestVersion ) {
setWpVersion( latestVersion.value );
}
}, [ wpVersions, setWpVersion ] );
Copy link
Contributor

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.

Copy link
Contributor

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 };
Copy link
Contributor

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

@katinthehatsite
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor

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.

Comment on lines 59 to 60
// find the index of the version that is less than the current version
const insertIndex = wordpressVersionOptions.findIndex( ( compareVersion ) => {
Copy link
Contributor

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":

Suggested change
// find the index of the version that is less than the current version
const insertIndex = wordpressVersionOptions.findIndex( ( compareVersion ) => {
const indexOfPrevVersion = wordpressVersionOptions.findIndex( ( compareVersion ) => {

Copy link
Contributor

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
Copy link
Contributor

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.

@bcotrim
Copy link
Contributor Author

bcotrim commented Mar 14, 2025

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.
I tried to be mindful of variables names and comments, but please let me know if you have any suggestions or notice any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants