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

Enhance/display important dates on search #41242

Conversation

millerf
Copy link
Contributor

@millerf millerf commented Apr 18, 2020

Changes proposed in this Pull Request

  • Depending on post/page status, display the most meaningful date
  • On search, display the said date with the status label
  • Small Good Refactor of PostRelativeTime

Search results should look close to this mockup:
Screenshot 2020-04-15 at 00 31 49

Testing instructions

  • Go to pages or posts views. Make sure displayed dates are the right ones
  • When searching, make sure the dates are the same and label is displayed

Extras

Found a bug reported here
#41245
As it is out of scope for this one.

Fixes #41075

cc @mmtr @sixhours

@millerf millerf changed the title WIP: Enhance/display important dates on search Enhance/display important dates on search Apr 18, 2020
return time.fromNow();
// 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( '[on] 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: 7
See 1 additional suggestions in the PR translation status page

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'LL [at] LT' has been modified and is now '[on] ll [at] LT'

statusText = this.props.translate( 'Publish immediately' );
}
const displayScheduleTime = this.getDisplayedTimeForLabel();
statusText = this.props.translate( 'scheduled %(displayScheduleTime)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 3 times:
translate( 'scheduled for %(displayScheduleTime)s' ) ES Score: 8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'scheduled for %(displayScheduleTime)s' has been modified and is now 'scheduled %(displayScheduleTime)s'

@millerf
Copy link
Contributor Author

millerf commented Apr 18, 2020

PostRelativeTime was refactored.

Posts and Pages views were modified according to specs:

Before After Comment
Screenshot 2020-04-18 at 19 21 32 Screenshot 2020-04-18 at 19 21 17 On listing, scheduled posts/pages now display the right date (scheduled ate)
Screenshot 2020-04-18 at 19 23 56 Screenshot 2020-04-18 at 19 25 06 Published now display "sticky" if needed
Screenshot 2020-04-18 at 19 29 19 Screenshot 2020-04-18 at 19 26 25 Post Search now displays the right status and appropriate date. The "sticky" status has been added. Maybe need some feedback from design?
Screenshot 2020-04-18 at 19 29 05 Screenshot 2020-04-18 at 19 31 03 Pages search now displays only one date and the right status

ramonjd and others added 2 commits April 20, 2020 12:01
…ow (Automattic#41129)

* initial commit:
- adding methods for start and end
- adding a constant for the flow name (could also be used for API calls, e.g., sites/new)

* refactored the start and complete to use a method with a default flow id
Changed the `gutenboarding` signup start track id with `calypso_signup_step_enter`

* Adding calypso_signup_complete tracking with new site and new user props. The current signup framework also passes `hasCartItems`, but I think we'll need a way to store that before we can pass it given that we do a redirect to the checkout

* Fix linting issue, add newUser to Header useEffect dependency array

* Update client/landing/gutenboarding/utils/analytics.ts

Correct jsdoc usage  comment

Co-Authored-By: roo2 <roo2@users.noreply.github.com>

* Remove useless comment.

* Moved analytics from /utils to /lib/analytics as utils seem to contain pure functions/no side effects. We can think about where to put analytics later, and whether to add to a store for easier testing similar to the Calypso analytics actions

Co-authored-by: Andrew Serong <andrewserong@users.noreply.github.com>
Co-authored-by: roo2 <roo2@users.noreply.github.com>
@mmtr mmtr requested a review from a team April 20, 2020 09:00
@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 20, 2020
Aurorum and others added 14 commits April 20, 2020 11:07
* Add suffix to railcar identifier
* Shift the railcar identifier logic up one level
…1264)

* Include the site id in `calypso_signup_complete` event.

Per request by p2-pau2Xa-12j, this commit adds the site id to
`calypso_signup_complete` event. In the same vein, `isNewSite` becomes a
derived data within the function itself, hence removing it from the call
sites.

* Update `site_id` as `blog_id` since the latter is what Tracks expects.
* Modularise login state

* Create directory for login actions

* Split out remoteLoginUser

* Split out loginUser

* Split out updateNonce

* Split out loginUserWithSecurityKey

* Split out loginUserWithTwoFactorVerificationCode

* Split out loginSocialUser

* Fix bad imports

* Split out createSocialUser

* Split out connectSocialUser

* Split out disconnectSocialUser

* Split out createSocialUserFailed

* Split out sendSmsCode

* Split out push-related actions

* Split out account type-related actions

* Split out formUpdate

* Sort exports alphabetically

* Reorganise 2fa push poll code

* Remove unnecessary auth-options inits

* Fix addReducer on login entry point

* Extract addReducerEnhancer into a util
…and DNS (Automattic#41019)

Show the destination of a domain - does it point to WordPress.com site or is external - in the "Change your name servers & DNS records" nav item
* Rework Selectors as class to manage `vendor`

* Provide suggestions vendor to store registration
* Jetpack Cloud: Scanner display threats

* Fixes issues with typing.

* Adds threat type parser.

* Cleans up unused code and adds handling for idle state.

* Moves some logic to description component.

* Fixes style bugs with spacing.

* Adds typing for threat fixing.

* Adds logic around fixers and additional typing.

* Fixes position of fixing buttons on mobile.

* Fix buttons layout on mobile

Co-authored-by: Echo G <ChaosExAnima@users.noreply.github.com>
Co-authored-by: Renzo Canepa <rcanepag@gmail.com>
* Add Earth Day Live setting toggle to settings

* Add moment import

* Update copy as per editorial feedback
@sixhours
Copy link
Contributor

Thanks @millerf ! It looks like this needs to be rebased, it's asking me to use npm when we recently switched to yarn. Give that a try and let us know if you need help with it!

Copy link
Contributor

@sixhours sixhours left a comment

Choose a reason for hiding this comment

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

This is not a comprehensive review. Please disregard if this is still "in progress"!

I noticed a couple issues when I fired this up:

When searching posts, the date shows "Published modified on [date]":
Screen Shot 2020-04-20 at 11 48 25 AM

When searching for a scheduled post, the line "Scheduled for Tomorrow at [time]" reads oddly because "tomorrow" is capitalized:
Screen Shot 2020-04-20 at 12 25 31 PM

The latter issue isn't unique to this PR (I noticed the same behavior on prod) but might be something we could address. Totally OK if that's out of scope and we address in a different PR: let me know and I'll create an issue.

@@ -211,7 +211,7 @@ class PostItem extends React.Component {
post={ post }
link={ enabledPostLink }
target={ null }
gridiconSize={ ICON_SIZE }
gridIconSize={ ICON_SIZE }
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but gridiconSize is correct, as Gridicon is one word: https://github.com/Automattic/gridicons

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn... I tried to look smart... 😄

@@ -62,7 +62,7 @@ function PageCardInfo( {
post={ page }
link={ contentLink.contentLinkURL }
target={ contentLink.contentLinkTarget }
gridiconSize={ ICON_SIZE }
gridIconSize={ ICON_SIZE }
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here re: Gridicon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revert the commit

link: PropTypes.string,
target: PropTypes.string,
gridiconSize: PropTypes.number,
gridIconSize: PropTypes.number,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved on #41301

@millerf
Copy link
Contributor Author

millerf commented Apr 20, 2020

"Published modified on [date]"

I see. What would be the correct way to say it... the idea is that the status of the post is published but the date is the last modified date.
Would "Published, last modified on [date]" be acceptable?

"Scheduled for Tomorrow at [time]"

It is definitely in the scope. I'll fix it.

I am having some trouble to push since I rebased. I don't think it is linked to the rebase, as I seem to have the same trouble on new branches...

refusing to allow an OAuth App to create or update workflow `.github/workflows/full-site-editing-plugin.yml` without `workflow` scope

So not sure if I would be able to address all that tonight.

@sixhours
Copy link
Contributor

Would "Published, last modified on [date]" be acceptable?

Yes, that sounds right to me!

It is definitely in the scope. I'll fix it.

Yay! Thank you!

I am having some trouble to push since I rebased. I don't think it is linked to the rebase, as I seem to have the same trouble on new branches...

That's a new one to me. 😬 It sounds like something weird with Git/Github? Hoping @kwight can offer some insight.

@millerf
Copy link
Contributor Author

millerf commented Apr 20, 2020

It sounds like something weird with Git/Github? Hoping @kwight can offer some insight.

It is bizarre as I am pushing on my fork... Also using command-line. All help I found online relate to git GUIs... I might just scratch the local repo...

@sixhours
Copy link
Contributor

I might just scratch the local repo...

That's my go-to for fixing weirdness, too. Burn it all to the ground. :D

@millerf millerf requested review from oandregal and a team as code owners April 20, 2020 18:08
@millerf
Copy link
Contributor Author

millerf commented Apr 20, 2020

I might just scratch the local repo...

That's my go-to for fixing weirdness, too. Burn it all to the ground. :D

Wow... I deleted the local repo, updated git, nothing worked. I ended up downloading Github Desktop and it worked straight away... Something about OAuth tokens that weren't saved the right way or something. Github desktop manages it in the automatically in the background and it worked...

@millerf millerf closed this Apr 20, 2020
@millerf millerf deleted the enhance/display_important_dates_on_search branch April 20, 2020 18:14
@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 20, 2020
@millerf
Copy link
Contributor Author

millerf commented Apr 20, 2020

Work continues on #41301

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.

Display only most important date when listing posts or pages