Skip to content
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

Posts: Improve Clarity of Scheduled Time #40843

Merged
merged 8 commits into from
Apr 13, 2020

Conversation

Aurorum
Copy link
Contributor

@Aurorum Aurorum commented Apr 7, 2020

Changes proposed in this Pull Request

This ensures that the scheduled time clearly states when the post is due to be released.

Testing instructions

A few things to check:

  • when the post is due to be released within a week, it shouldn't give the full date, but it should give the time (I would find this very helpful as someone who tries to schedule posts weekly at the same time, so I'm sure many others would too)
  • when the post is due to be released after a week, it should give the full date
  • "scheduled in" should be replaced with "scheduled for" (the latter gave the impression that was when the "Schedule" button was actually clicked, when that isn't what it conveys)

Before:
Screenshot 2020-04-07 at 11 58 36

After:
Screenshot 2020-04-07 at 12 08 36

Note: I made a few changes so that it displays the date and then "at" (eg. 27 April 2020 at 02:00) so that it's consistent with the wording for a post due to be published in under 7 days.

Follows up to #40779 and #40584
Fixes #12052

? scheduledDate.calendar()
: scheduledDate.format( 'LL' ) +
' ' +
this.props.translate( 'at', {
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 3 times:
translate( 'at', { context: 'time comment was posted'} ) ES Score: 7
See 1 additional suggestions in the PR translation status page

ℹ️ This context is really long. Are you sure you don't want to use a translator comment instead?

@mmtr mmtr requested a review from a team April 7, 2020 11:52
@mmtr mmtr added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 7, 2020
@sixhours
Copy link
Contributor

sixhours commented Apr 7, 2020

This is so much clearer. 👍 /cc @Automattic/dotcom-manage-design because I worry the strings are too long for a relatively small space.

I wonder if we shortened it to something like "Scheduled for Feb 5" with a tooltip that specified the exact time? That's not particularly elegant, though...

@Aurorum
Copy link
Contributor Author

Aurorum commented Apr 7, 2020

The months could be shortened (eg. April -> Apr) like the text before it. But, to be honest, I'm not sure that'll make a great difference. :)

Screenshot 2020-04-07 at 13 55 47

' ' +
scheduledDate.format( 'LT' );

statusText = this.props.translate( 'scheduled for %(scheduledTime)s', {
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 1 times:
translate( 'scheduled %(scheduledTime)s' ) ES Score: 7

Aurorum and others added 2 commits April 7, 2020 16:50
Co-Authored-By: Alex Kirk <akirk@users.noreply.github.com>
@millerf
Copy link
Contributor

millerf commented Apr 7, 2020

I worry the strings are too long for a relatively small space

It definitely seems a bit crowded on small screens.

Screenshot 2020-04-07 at 21 34 35

I think the year is a useless piece information, unless it is more than one year in the future. If you are in December 2020, and it says "scheduled for Jan, 5th". It is straightforward that means January 2021...

@Aurorum
Copy link
Contributor Author

Aurorum commented Apr 7, 2020

Thanks for looking at this! I agree that hiding the year if the post is due to be released within 365 days is an easy way to cut some space! I've made that change now. :)

With a post due to be released within a year:

Screenshot 2020-04-07 at 21 32 37

With a post due to be released after a year:

Screenshot 2020-04-07 at 21 36 40

@gwwar
Copy link
Contributor

gwwar commented Apr 7, 2020

@sixhours and @Automattic/dotcom-manage-design I'm not sure we need to display multiple dates in this case.

Looking back at wp-admin the date shown is contextual to the post status. For published, we show last publish date, for drafts last modified, and for scheduled the most important piece of information is when it will be published in the future vs the last edit. There's a bit of ambiguity in Calypso, with the icon over a text label, which is maybe why we tried showing both?

Screen Shot 2020-04-07 at 2 27 34 PM

const now = moment();
const scheduledDate = moment( this.props.post.date );
// If the content is scheduled to be release within a year, do not display the year at the end
const displayDate = scheduledDate.diff( now, 'years' ) > 0 ? 'll' : 'D MMM';
Copy link
Member

Choose a reason for hiding this comment

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

D MMM assumes that languages use a day-month format but some use month-day. Couldn't we rather replace the Y from the generated ll? Something like this:

if ( scheduledDate.diff( now, 'years' ) < 1 ) scheduledTime.replace( scheduledDate.format( 'Y' ), '' );

Copy link
Member

Choose a reason for hiding this comment

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

@mmtr could we please get this addressed also? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed! :)

@millerf
Copy link
Contributor

millerf commented Apr 8, 2020

There's a bit of ambiguity in Calypso, with the icon over a text label,

@gwwar It has always been misleading on calypso. Mainly because the icon is always the same, so there is no way to distinguish which date is being displayed.
That was my aim on this issue: if I remember well, the "modified", "published" and "future" dates were used (but only on "pages"), "post" weren't using the "published" dates.

Now that "posts" and "pages" are using the same component to display this information, it will be way easier to fix it, although it goes out of the scope of this very issue, and we definitely need some guideline from @sixhours to know where we are headed. Maybe getting slightly different icons to carry the ideas of "last modified", "published on" and "scheduled for" would be enough?

Idea Icon
Last modified Screenshot 2020-04-08 at 08 07 12
Published ca_published
Scheduled for ca_scheduled

@sixhours
Copy link
Contributor

sixhours commented Apr 8, 2020

For this PR, I like that the post date is clear about what time it is scheduled for.

I agree with @gwwar that an easy fix to reduce the amount of copy is to remove the "last modified" date entirely and only display the date it's published, or the date it's scheduled.

But I don't know how that stacks up against past changes, I'm coming into this in the middle. Apologies if I'm confusing things even more. :)

@gwwar
Copy link
Contributor

gwwar commented Apr 8, 2020

I agree with @gwwar that an easy fix to reduce the amount of copy is to remove the "last modified" date entirely and only display the date it's published, or the date it's scheduled.
But I don't know how that stacks up against past changes, I'm coming into this in the middle. Apologies if I'm confusing things even more. :)

Would you like to pick @lcollette? I'm in favor of dropping the less important date, so we can have a proper text label. (I always find mystery icons without a text label to be super confusing). Happy to defer here though if y'all had other thoughts.

@lcollette
Copy link
Contributor

I agree with @gwwar that an easy fix to reduce the amount of copy is to remove the "last modified" date entirely and only display the date it's published, or the date it's scheduled.

I also agree with removing the last modified date entirely. The date it's published, or scheduled to publish, seems more useful.

lcollette
lcollette previously approved these changes Apr 9, 2020
@mmtr
Copy link
Member

mmtr commented Apr 10, 2020

I also agree with removing the last modified date entirely. The date it's published, or scheduled to publish, seems more useful.

@lcollette should we remove the last modified date then on this PR? Or are you suggesting to do it as a follow-up?

@gwwar
Copy link
Contributor

gwwar commented Apr 10, 2020

@lcollette should we remove the last modified date then on this PR? Or are you suggesting to do it as a follow-up?

I think either would be fine provided the follow up PR is relatively quick to land after.

@mmtr
Copy link
Member

mmtr commented Apr 13, 2020

@Aurorum This should be ready to go after addressing the linting issues and taking into consideration the current locale date format when removing the year from the datetime.

@mmtr mmtr dismissed lcollette’s stale review April 13, 2020 10:10

Just making sure we don't merge this before addressing the pending issues

const scheduledDate = moment( this.props.post.date );
// If the content is scheduled to be release within a year, do not display the year at the end
const scheduledTime = scheduledDate.calendar( null, {
sameElse: this.props.translate( 'll [at] LT', {
Copy link

Choose a reason for hiding this comment

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

ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 20 times:
translate( 'LL [at] LT' ) ES Score: 12

@Aurorum
Copy link
Contributor Author

Aurorum commented Apr 13, 2020

I think both should now be fixed, thanks @mmtr! :)

Copy link
Member

@mmtr mmtr left a comment

Choose a reason for hiding this comment

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

Thanks @Aurorum! This tests well for me.

Before After
Screen Shot 2020-04-13 at 14 46 40 Screen Shot 2020-04-13 at 14 46 47

We can possibly use a sentence case notation when the scheduled date is today/tomorrow in order to avoid a capitalized Today/Tomorrow, but not a blocker here.

@mmtr mmtr merged commit d5304d7 into Automattic:master Apr 13, 2020
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 13, 2020
@mmtr
Copy link
Member

mmtr commented Apr 14, 2020

Filed #41075 to follow up on removing the last modified date on scheduled posts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scheduled date shown in post list is not useful
9 participants