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

fix(desk): edit logic for published- and edit status message in document list #5067

Merged
merged 4 commits into from
Oct 31, 2023

Conversation

ninaandal
Copy link
Contributor

@ninaandal ninaandal commented Oct 25, 2023

Description

The logic for published and edits message in the document list was wrong and would be incorrect in certain instances.
This was also the case for the PublishAction tooltip.

Before:
Screenshot 2023-10-25 at 11 14 59
Screenshot 2023-10-25 at 11 14 54
Screenshot 2023-10-27 at 13 38 59

Instead of using <TimeAgo />, we can use useTimeAgo() and show label with abbreviations and suffix.

What to review

The document list published and edits status.

Notes for release

Fixed published and edits message in document list

@vercel
Copy link

vercel bot commented Oct 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview Oct 30, 2023 11:50am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 30, 2023 11:50am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Oct 30, 2023 11:50am

@ninaandal ninaandal requested review from a team October 25, 2023 09:29
@github-actions
Copy link
Contributor

No changes to documentation

@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2023

Component Testing Report Updated Oct 30, 2023 11:54 AM (UTC)

File Status Duration Passed Skipped Failed
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 10s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 11s 3 0 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 11s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 31s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 15s 9 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 59s 18 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 11s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 7s 3 0 0

@ninaandal ninaandal changed the title fix(desk): edit logic for published- and change status message in document list fix(desk): edit logic for published- and edit status message in document list Oct 25, 2023
@robinpyon
Copy link
Contributor

Great find @ninaandal

Since this issue lies with the <TimeAgo> component – perhaps it makes sense to find other places that's used (I think there are just 1-2), apply this same fix and then remove that component altogether?

@bjoerge bjoerge requested review from bjoerge and removed request for a team October 27, 2023 11:21
Copy link
Member

@bjoerge bjoerge left a comment

Choose a reason for hiding this comment

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

Nice, good catch & thanks for fixing!

@ninaandal
Copy link
Contributor Author

Great find @ninaandal

Since this issue lies with the <TimeAgo> component – perhaps it makes sense to find other places that's used (I think there are just 1-2), apply this same fix and then remove that component altogether?

There are several <TimeAgo /> components and I think it makes sense to keep as is for the preview components (reference and CDR reference) as this shows how long ago it was (like almost two years ago instead of the date it was published). But I'll remove the one that is used for the PublishAction as this can be replaced with useTimeAgo()

Copy link
Contributor

@robinpyon robinpyon left a comment

Choose a reason for hiding this comment

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

Thanks @ninaandal!

I think it makes sense to keep as is for the preview components (reference and CDR reference) as this shows how long ago it was (like almost two years ago instead of the date it was published).

(nitpicking and not blocking) I would opt for consistency here – as rolling over these publish icons in different contexts currently yields different values (document list previews say "published yesterday", whilst reference inputs say "published 1 day ago"). Whilst we may know they're different components, it may not be apparent to users.

Screenflick.Movie.15.mp4

@ninaandal
Copy link
Contributor Author

Thanks @ninaandal!

I think it makes sense to keep as is for the preview components (reference and CDR reference) as this shows how long ago it was (like almost two years ago instead of the date it was published).

(nitpicking and not blocking) I would opt for consistency here – as rolling over these publish icons in different contexts currently yields different values (document list previews say "published yesterday", whilst reference inputs say "published 1 day ago"). Whilst we may know they're different components, it may not be apparent to users.

Screenflick.Movie.15.mp4

We should make a decision of which format we would like then, as the reference preview one uses date-fns/formatDistanceToNow to format, while the document list items does not.
Do we want it to have the format: Published about 1 years ago for example, or just show the date when it was published (see the two screenshots)?
Maybe @kaylasanity wants to chime in here?

Screenshot 2023-10-30 at 09 25 32
Screenshot 2023-10-30 at 09 25 24

@ninaandal
Copy link
Contributor Author

Had a chat with @kaylasanity and decided to go for the timestamp format ✨

@ninaandal ninaandal added this pull request to the merge queue Oct 31, 2023
Merged via the queue into next with commit 2013203 Oct 31, 2023
28 checks passed
@ninaandal ninaandal deleted the edx-670 branch October 31, 2023 09:14
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