Skip to content

Conversation

@ingeniumed
Copy link
Contributor

@ingeniumed ingeniumed commented Sep 20, 2024

Description

  • Add support for date time as an editorial metadata field. This mirrors the way gutenberg shows the publish time of a post, which allows it to not clutter up the sidebar.
  • Change the fields offered out of box, to be unique to our plugin - Embargo Date, Expiry Date and Legal Review.
  • Notification test that tested the cron code was flaky so I removed it. Need to re-think what that would look like

Screenshot 2024-09-20 at 3 35 16 pm

Steps to Test

  • Delete the editorial metadata terms that are on your local env
  • Delete the vw_editorial_metadata_options option from your local env
  • Ensure the new ones are created from scratch
  • Test out the new date time picker

@ingeniumed ingeniumed self-assigned this Sep 20, 2024
@ingeniumed ingeniumed requested a review from a team as a code owner September 20, 2024 05:54
@alecgeatches
Copy link
Contributor

@ingeniumed This works great! I'm close to approval, but have a handful of UI suggestions for polish. These range from most to least important to me, so feel free to push back if you think we should save this for another pass.

  1. Month arrows in the EM date popover don't stay in a consistent location. For example, when selecting a post publish date, these stay fixed:

    post-date-alignment


    However, with our custom popover, these sometimes move:

    em-month-alignment

    This may be a trickier change to implement, but I think it's important to make this control work well.

  2. There are a handful of elements from the publish date control that feel missing:

    publish-date-control

    1. The "Now" button is super useful, especially if you're updating the time something happened. I see that the date defaults to now if there's no value, but if anything has changed or the value is edited this convenience is missing. Entering the current date and time manually is doable but slow.
    2. An "x" button. I was looking for a way to save or close the date picker, and it felt awkward to click outside the popover and hope my data was saved. We could also have a "Save" or "Close" button at the bottom for the same effect.
    3. AM/PM. These aren't strictly necessary for 24-hour time, but because the publish date box has these it isn't clear that we've switched to 24-hour time, it just looks like they're missing.
  3. The date formatting should probably match WordPress' default format. Here's the publish date:

    publish-date-format

    and an EM-defined date:

    em-date-format

    If we're not selecting the EM date format for a particular reason, it should probably just match.

  4. I don't see a good way to empty a date if I want to clear out the value. This doesn't matter so much for the publish date component, as you don't have to publish at all if you don't want. However, if I want to clear out a previously set date, I don't see any way to do that. Maybe we could provide a button in the dialog or sidebar to "Clear date".

@ingeniumed
Copy link
Contributor Author

Great feedback, and I've made all those changes. As for the inconsistent arrows, it seems to have fixed itself by adding in the AM/PM toggles.

What I did extra:

  • Hardcoded the style on the dropdown so that it's correctly aligned. Previously, the first element was never aligned which was really infuriating. This is a setting on the dropdown that we cannot override, likely because of when our CSS loads.
  • Hardcoded the style on the date dropdown button so that the text would wrap correctly. This is again, another CSS that loads after ours loads so it's overwritten.

In case there's a better fix than what I did, lemme know. Otherwise, this should be good to go.

@ingeniumed
Copy link
Contributor Author

ingeniumed commented Sep 23, 2024

Screenshot 2024-09-23 at 3 07 20 pm - this has the old misaligned CSS so ignore that bit. Just focus on the popover.

Screenshot 2024-09-23 at 3 25 12 pm

@jarekmorawski
Copy link

Love all the suggestions here. Great job, folks! These small changes will undoubtedly improve the editing UX. From the UI point of view, I wonder if we could try to make the metafields even more integrated than they're now.

Here's what I have in mind:

  • extend the default status field instead of adding a custom one
  • move all metafields to the Summary section in the Post tab in the sidebar
  • add a separator to make them stand out a bit
  • display the desc/help text in a tooltip to reduce the visual footprint
  • tweak their appearance to match other post props in the sidebar
  • show some controls (like toggles, checkboxes) inline, and others in popovers, similar to core

Default

This way the metafields would be extremely easy to find and interact with because they'd use interfaces the users are already familiar with. Furthermore, should Gutenberg ever update that UI, we'd automatically benefit from it and upgrade the editing UX in Workflow.

Status

Status

Checkbox

Checkbox

Input

Input

Select

Select

Date

Date

Please let me know what you think! There's probably nuance I'm missing, but I think we should aim for aligning with core as much as possible.

@ingeniumed
Copy link
Contributor Author

ingeniumed commented Sep 23, 2024

Thanks for the wonderful suggestions, and the mocks for each of em @jarekmorawski!

extend the default status field instead of adding a custom one

This was something that we left in place from Edit Flow, as custom status support in WP isn't 100%. This field is likely going to go away, once we finish implementing role based status restrictions. There'd be a way for admins to bypass that restriction. If we do decide to keep it, we can do some experimentation to see if we can inject into that existing status field.

move all metafields to the Summary section in the Post tab in the sidebar
add a separator to make them stand out a bit
display the desc/help text in a tooltip to reduce the visual footprint
tweak their appearance to match other post props in the sidebar
show some controls (like toggles, checkboxes) inline, and others in popovers, similar to core

Agreed with all this. It would def make the appearance of everything a lot cleaner, and would provide a consistent appearance across versions.

I think we should aim for aligning with core as much as possible.

100%! That is our goal - do it the way core does it as much as possible.

@ingeniumed
Copy link
Contributor Author

Screenshot 2024-09-24 at 9 05 11 pm
Screenshot 2024-09-24 at 9 05 16 pm

Got everything done with one todo for a future PR:

See if we can improve the alignment of the checkbox with the date, and text. It's close to perfect but just a touch off. We can do this in another PR in the interest of getting this in and moving on to making the EM fields required.

Copy link
Contributor

@alecgeatches alecgeatches left a comment

Choose a reason for hiding this comment

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

@ingeniumed I love the design updates and pop-outs for data! It works really well. I did notice a few UI issues (mostly alignment) other than the text on the sidebar, and I just wanted to note those for posterity with the understanding that they'll be addressed in another PR. Here are the main issues I saw:

  1. The "text"-type pop-out seems like it's a textarea where a regular text input might fit better. For example, here's the "Author" pop-out from native WordPress:

    author-pop-out

    Compare this with the "Word Count" pop-out:

    Screenshot 2024-09-24 at 1 45 30 PM

    A textarea-sized input doesn't feel right here, especially for something like a word count. I'd suggest we make these all one-line inputs, as we don't really have a lot of space to store paragraphs of text in the UI. Alternatively, we could have a separate input type for short and long inputs. I'd prefer we just use short everywhere though.

  2. The alignment of text labels seems a little off. Here's "Assignment" versus "Word Count":
    Screenshot 2024-09-24 at 1 45 25 PM


    Screenshot 2024-09-24 at 1 45 30 PM


    "Word Count" seems squeezed into the corner. This is probably just a flex issue that we can fix so that it's center-aligned and uses the available horizontal space.

  3. Even on a large-sized screen, it's easy to show scrollbars on the calendar component. It's dependent on the current scroll, but always seems to happen if the sidebar is scrolled to the bottom:

    2024-09-24 14 03 39

    Ideally we can render the component so that scrollbars appear in fewer circumstances as they make the calendar very awkward to use. Also, note the date string escaping the button on the sidebar as well:

    Screenshot 2024-09-24 at 1 58 18 PM


As mentioned above, we can get these in the next pass. Thanks Gopal!

@ingeniumed
Copy link
Contributor Author

Thanks for pointing all those issues out, it's good to note em down so they can be fixed in another PR.

Before I merge this, I'll switch the textareControl with a textControl as you are right about that. I had gone back and forth on that, and settled on textareaControl just in the interest of the space available. But, all good we can revisit the alignment, etc on that later.

@ingeniumed ingeniumed merged commit fc67a3b into trunk Sep 24, 2024
@ingeniumed ingeniumed deleted the add/date-type-support branch September 24, 2024 21:12
@jarekmorawski
Copy link

This was something that we left in place from Edit Flow, as custom status support in WP isn't 100%.

Is there anything we can do to make it happen in core? Would it make sense to set aside time to solve this problem there and unlock a better UX for our plugin? I know the issue has been there for years and it's high time to do something about it (knowing WP, there's probably a PR already but didn't get enough traction to be merged).

There'd be a way for admins to bypass that restriction. If we do decide to keep it, we can do some experimentation to see if we can inject into that existing status field.

Some user roles should be able to bypass and edit statuses. One I can think of is the role that can create and edit custom statuses in the plugin. Did we specify what role that would be? Should we let site admins decide?

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