Post Schedule: show post events #29716
Conversation
|
Size Change: +1.43 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
retrofox
force-pushed
the
update/show-existing-posts-in-date-picker
branch
2 times, most recently
from
863f613
to
9f2c56f
Compare
|
I believe this meta post might have some relevance to the PR. |
Yeah, which is nice! 😅 The app requests the posts after the date picker is rendered, once. If you change the month and go back to it, the delay won't happen. |
|
@tomjn could you take a look at this one too? I saw you have been reviewing similar suggestions lately. Thanks in advance 🙇♂️ |
Yes definitely! We could adjust the API call to bring back post revisions with a future status too. That got me wondering if we should include a way to let this be extended by plugin developers etc. Some way of adjusting the events data and how they're displayed. Not something for this PR though! |
|
This is looking good, but as this will eventually be shipped to all WordPress sites, I think we should make sure that there's a way of disabling it, or some other sensible default, for sites that are posting every day. Having every date ringed (and eventually with tooltips) could be more of an annoyance than an aid. Another thought would be to make the indicator more subtle, like the Basecamp dot calendar.
But I'm not sure how we'd handle having many posts on a single day and this kind of redesign seems beyond the scope of this change. |
|
That looks like a nice enhancement for another ticket.
I took a look at the code but didn’t have a chance to test it.
My one area of uncertainty was timeline related, afterall 1am UK time on
the 25th, is actually the 24th for someone in USA eastern time, and if I
published a post from Tokyo or New Zealand the chances are even higher
…On Fri, 12 Mar 2021 at 11:48, Paul Bunkham ***@***.***> wrote:
This is looking good, but as this will eventually be shipped to all
WordPress sites, I think we should make sure that there's a way of
disabling it, or some other sensible default, for sites that are posting
every day. Having every date ringed (and eventually with tooltips) could be
more of an annoyance than an aid.
Another thought would be to make the indicator more subtle, like the
Basecamp dot calendar.
[image: image]
<https://user-images.githubusercontent.com/96462/110936224-759bb000-8328-11eb-96fc-8fde4df78d79.png>
But I'm not sure how we'd handle having many posts on a single day and
this kind of redesign seems beyond the scope of this change.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#29716 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAOLZ6BA7M7YZXAK6TPHADTDH5PDANCNFSM4Y54NKKA>
.
|
I've added a toggle control to show/hide the calendar events. |
Yes, scheduling based on time zone is a tricky problem, but this is more an interface to the current functionality and the times will be displayed in the current user's timezone. |
Cool. I've added the design feedback label because it would be good to get some input as to whether this is the best solution to the problem of a noisy calendar, and whether the toggle needs any tweaks. I was wondering about the term 'events'. It's more obvious what it means when you see them highlighted, but I don't know if it's the best word. Having said that, I'm struggling to come up with anything else! |
|
This is what I see:
Nice job Damian! With some experimenting I see various posts that have been published as well as scheduled published posts. I believe we need some more people to take a closer look here. I searched for calendar issues and see that @retrofox Damian has gone through and taken feedback from a few of these and added it to the PR. It would be nice with some design folks to take a look at this issue. |
Oh I understood that was the intention, it was my expectation too :) I just didn't know if it was handled or not from a read of the code, it'll need someone to confirm it by testing |
|
[Design review based off of screenshots] I love this. One of my sites is is one that I try to post daily to and its very frustrating to try and remember which day doesn't have a post and which does - especially when I'm scheduling it in advance. I would love to see a solid month of every date highlighted! Means I'm doing something right - if my job is to post daily. And on the sites I have that I barely update, this will be a good indicator of when I last posted. I don't think this toggle should be local to the page/post. I would argue against having the setting at all, but if others have a specific use case or if we get a ton of complaints: add it in a settings page so it is site wide and out of the way here. Having a dot to represent events on calendars is very standard practice at this point, as @pablinos mentioned. I think it would be more subtle, yet still quite useful. And then we can remove that toggle :)
|
retrofox
force-pushed
the
update/show-existing-posts-in-date-picker
branch
from
18a058e
to
cd22e14
Compare
I thought about Do you have some suggestions for re-wording it? |
|
Updated :-)
|
retrofox
force-pushed
the
update/show-existing-posts-in-date-picker
branch
from
f07d4bd
to
3a9bc18
Compare
Thanks @gwwar for your feedback and help. I've updated the aria-label hack to make it more solid. It's in a better place to go imo. Tested with a screen reader again, just in case: post-schedule-events-a11y.mp4Finally, e2e tests are passing which is great. Thanks again 🙇♂️ |
|
|
||
| parentNode.setAttribute( | ||
| 'aria-label', | ||
| `${ dayAriaLabel }. ${ eventsString }` |
There was a problem hiding this comment.
Is it possible to make this a single translation string instead of a concat on two translated strings together?
Translators get more context this way and it avoids a poor translation, especially if things would be phrased differently together.
There was a problem hiding this comment.
<3. I think it's handled here f677f37
There was a problem hiding this comment.
Thanks for your work here @retrofox, folks will enjoy this one!
Giving tentative approval, but I'd like a +1 on the code approach. Perhaps @youknowriad could take another 👀
This manually tests well for me. Here's a screenshot of what the screen reader controls look like:
| } | ||
|
|
||
| const { parentNode } = ref.current; | ||
| const dayAriaLabel = moment( day ).format( ARIAL_LABEL_TIME_FORMAT ); |
There was a problem hiding this comment.
Something I noticed and better left for a follow up PR:
This is one of the few usages of moment in the project. It looks like we might have some options using @wordpress/date or even native browser support if that eventually falls into WP support ranges:
| @@ -4,6 +4,7 @@ | |||
| // Needed to initialise the default datepicker styles. | |||
| // See: https://github.com/airbnb/react-dates#initialize | |||
| import 'react-dates/initialize'; | |||
| import { noop } from 'lodash'; | |||
There was a problem hiding this comment.
For other reviewers, are we trying to reduce lodash usage in the project?
| () => | ||
| ( eventsByPostType || [] ).map( | ||
| ( { title, type, date: eventDate } ) => ( { | ||
| title: title?.raw, |
There was a problem hiding this comment.
Do folks know if we need the raw or rendered value 🤔
There was a problem hiding this comment.
I wasn't sure either and since it isn't going to be consumed on this approach, I preferred using the raw version of the title. However, if we'd like to implement the events tooltip we may like to go for the rendered version to format the event list:
There was a problem hiding this comment.
Seems like this should be the rendered title, the raw is used when editing only AFAIK
| ); | ||
|
|
||
| parentNode.setAttribute( 'aria-label', dayWithEventsDescription ); | ||
| }, [ ref, events.length ] ); |
There was a problem hiding this comment.
I don't really understand why we're accessing and modifying attributes using DOM APIs here. I'm pretty sure you can get React warnings when doing things like that to Dom elements controlled by React.
There was a problem hiding this comment.
Also, using a "ref" as dependency is useless, a ref never changes its instance.
There was a problem hiding this comment.
I don't really understand why we're accessing and modifying attributes using DOM APIs here.
Because the component API doesn't support extending the aria-label attribute in a super flexible way. It's nice when setting the date format, which is great. We can use, and we do, the dayAriaLabelFormat prop:
<DayPickerSingleDateController
dayAriaLabelFormat="dd"
/>It generates the following markup (Saturday day):
<td aria-label"Sa" ...>Even is possible to set the property using arbitrary text, something like...
<DayPickerSingleDateController
dayAriaLabelFormat="dd [🦊 retrofox]"
/>
However, it isn't enough for our requirements, since what we'd like to achieve is setting a custom aria-label for each rendering day. Something like the following:
<DayPickerSingleDateController
dayAriaLabelFormat={ ( day ) => {
const dayAriaLabel = moment( day ).format( ARIAL_LABEL_TIME_FORMAT );
const events = this.getEventsPerDay( day );
return sprintf(
// translators: 1: Calendar day format, 2: Calendar event number.
_n(
'%1$s. There is %2$d event.',
'%1$s. There are %2$d events.',
events.length
),
dayAriaLabel,
events.length
);
} }
// ....
/>However, dayAriaLabelFormat only accepts a string. :'(
I'm pretty sure you can get React warnings when doing things like that to Dom elements controlled by React.
I think everybody agrees with that. The approach is quite far away to be ideal, but it works and it shouldn't hurt in the sense the app shouldn't get broken. In the worse cases, it should perform any change in the markup.
Our issue is not being able to extend the component as we wish, and AFAIK, DOM manipulation is the only solution without changing the react-dates component, something that we researched a few months ago.
There was a problem hiding this comment.
I do expect this to break often though, React might restrict us from doing that. Do we have warnings now in the console because of that?
There was a problem hiding this comment.
All warnings that I see belong to the react-dates component. They already are in the trunk branch. I don't see any about the DOM manipulation process.
The ref nevers changes its value
I think the a11y support worths it.
No, it doesn't seem to be possible. |
|
Thanks folks for your help. <3 |
|
thank youuuuuuu! |
Description
This PR collects the site posts and shows them as calendar events in the post schedule component.
Also, it adds theeditor.CalendarEventsfilter to be able to extend the events via a hook (doc updated):fixes #13713
How has this been tested?
Storybook
> npm run storybook:devhttp://localhost:50240/. Search the date-time component, or simply use this URL: http://localhost:50240/?path=/story/components-datetimepicker--with-days-highlightedAdd random highlightbutton. You should see how the calendar is populated with random events (circles with background yellow color)Editing a post
** Be sure some posts belong to the post that you are going to use to test the post schedule.
Videos
it updates the aria-label for each day to speak the events number, in case they exist.
post-schedule-events-a11y.mp4
Types of changes
Checklist: