-
Notifications
You must be signed in to change notification settings - Fork 0
Feature: #16 - Sanitise version number #24
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
base: main
Are you sure you want to change the base?
Conversation
| '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', |
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.
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' ), |
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.
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'; ?>"> |
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.
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; |
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.
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.
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: