Skip to content

Conversation

@ingeniumed
Copy link
Contributor

Description

This PR is meant to update the design for configuring the required fields on a status, using the mockups proposed by @jarekmorawski. Check out #44 for an example of this.

The changes I've made:

  • Implement a radio control for turning the user restriction on and off.
  • Change the labels to be clearer and drop the help labels when they aren't necessary.

Note: The rules engine is not in place for editorial metadata just yet. Once it is, that section would change a bit more so keep that in mind when reviewing this.

Screenshot 2024-10-02 at 12 07 35 pm
Screenshot 2024-10-02 at 12 07 45 pm
Screenshot 2024-10-02 at 12 07 52 pm
Screenshot 2024-10-02 at 12 07 58 pm
Screenshot 2024-10-02 at 12 08 37 pm

Steps to Test

  • Update and Create a status with editorial metadata fields, as well as users to see the difference.

@ingeniumed ingeniumed requested a review from a team as a code owner October 2, 2024 02:12
@ingeniumed ingeniumed self-assigned this Oct 2, 2024
const [ isRequesting, setIsRequesting ] = useState( false );
const [ isRestrictedSectionVisible, setIsRestrictedSectionVisible ] = useState(
requiredUsers.length > 0 || requiredMetadatas.length > 0
const [ areRestrictedUsersSet, setAreRestrictedUsersSet ] = useState(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not the biggest fan of setting text here to control the radio control, so happy to change this. This seemed like the best way imo for now.

@ingeniumed
Copy link
Contributor Author

Note: I've dropped roles from the verbiage but the screenshots still reflect the old verbiage. Just noting that, as we will add roles support in the future.

@jarekmorawski
Copy link

jarekmorawski commented Oct 2, 2024

It's looking good! The only part I'm unsure of is the required metafields label. It isn't clear to me what the field does. Would it make sense to change it to something like below?

image

Note that it's the inversion of the logic suggested in the new design where users would set visibility requirements on the succeeding status (e.g., 'Legal review' would only be visible if the 'Needs legal review' metafield is filled), instead of the current one (e.g., only move forward if X). I can't say if any of the options is better in the long term, but it does impact the UX and may cause potential complications later on. cc @smithjw1

@ingeniumed
Copy link
Contributor Author

ingeniumed commented Oct 2, 2024

Something I had gone back and forth on, was keeping that help text as it makes it clear. I ended up removing that help text in favour of the label being descriptive enough. I'll add it back in, and shift the required metadata fields above the users. Given the user input is hidden, it'll prevent the metadata fields from being pushed down.

We would want to keep the users and metadata fields requirement being applied on the current status rather than the next one, as that keeps the whole thing simple.

@ingeniumed
Copy link
Contributor Author

Screenshot 2024-10-02 at 8 03 50 pm

@ingeniumed ingeniumed merged commit b54f761 into trunk Oct 2, 2024
@ingeniumed ingeniumed deleted the update/design-for-required-fields branch October 2, 2024 10:31
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.

3 participants