Skip to content

Conversation

@HarryC05
Copy link
Collaborator

@HarryC05 HarryC05 commented Feb 3, 2025

Description

Fixes #16

This PR replaces the version input control with number controls, and a select control for pre-release titles.

Change Log

  • Replaces input control with multiple number controls and a select control
  • Removes is pre release meta
  • Adds schema to version meta

Steps to test

  1. create a new release
  2. input the version number
  3. publish the release
  4. see the correct version number in the admin bar, dashboard widget and on the release notes list

Screenshots/Videos

Screenrecording
http://bigbite.im/v/WfdR9h

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@HarryC05 HarryC05 requested review from a team and scottblackburn as code owners February 3, 2025 17:13
'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',
Copy link
Member

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' ),
Copy link
Member

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'; ?>">
Copy link
Member

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;
Copy link
Member

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:

  1. Extract splitVersion into 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.
  2. Move existing logic into an opposite joinVersion function to transform a version string into an object, for the same reason. It encapsulates that logic and gets it out of your component
  3. 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
  4. (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.

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.

Sanitise the version number on release notes

2 participants