Open
Conversation
g-elwell
requested changes
Feb 18, 2025
| 'parent' => 'top-secondary', | ||
| 'meta' => [ | ||
| 'class' => $is_pre_release ? 'release-note is-pre-release' : 'release-note', | ||
| 'class' => str_contains( $version, '-') ? 'release-note is-pre-release' : 'release-note', |
Member
There was a problem hiding this comment.
Although you're dropping the meta, I would recommend keeping a boolean variable in place, i.e: $is_pre_release = str_contains( $version, '-'); as it helps make the intent clearer.
| 'text' => [ | ||
| 'type' => 'plain_text', | ||
| 'text' => get_post_meta( $id, 'is_pre_release', true ) ? __( 'New Pre-Release 🎉', 'release-notes' ) : __( 'New Release 🎉', 'release-notes' ), | ||
| 'text' => str_contains( $version, '-' ) ? __( 'New Pre-Release 🎉', 'release-notes' ) : __( 'New Release 🎉', 'release-notes' ), |
Member
There was a problem hiding this comment.
Same as above, although this wasn't in place originally it may be an improvement to have a variable $is_pre_release defined here
| ?> | ||
|
|
||
| <article class="release-note-single<?php $is_pre_release && print ' is-pre-release'; ?>"> | ||
| <article class="release-note-single<?php str_contains( $version, '-') && print ' is-pre-release'; ?>"> |
Member
There was a problem hiding this comment.
Same as above, it may be an beneficial to keep the variable $is_pre_release defined here
| const { useEntityProp } = wp.coreData; | ||
| const { useSelect } = wp.data; | ||
| const { __ } = wp.i18n; | ||
| const { useState, useEffect } = wp.element; |
Member
There was a problem hiding this comment.
There are a few steps I'd recommend going through to improve the organisation of this JS code, and reduce complexity/chance of bugs:
- Extract
splitVersioninto a standalone function, defined outside of the React component (perhaps in a utils folder) This will force the removal of closures and side effects, clarifying that the function takes in X and returns Y. This makes it easier to understand, maintain and test. - Move existing logic into an opposite
joinVersionfunction to transform a version string into an object, for the same reason. It encapsulates that logic and gets it out of your component - Now the component is cleaner, it should be easier to see that some state/effects are unnecessary.
const versionObject = splitVersion(version);will achieve the same result you have here, as we're just calculating the version object based on existing props/state - https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state - (optional) consider grouping the multiple input fields for the version into a dedicated component, ie
<VersionControl value={version} onChange={handleChange} />which can accept and return a version string - handling the conversions internally.
This file contains hidden or 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
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
Fixes #16
This PR replaces the version input control with number controls, and a select control for pre-release titles.
Change Log
Steps to test
Screenshots/Videos
Screenrecording
http://bigbite.im/v/WfdR9h
Checklist: