Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

Update Pashto locale date format for LL & LLL #3357

Merged
merged 3 commits into from
Apr 9, 2020
Merged

Update Pashto locale date format for LL & LLL #3357

merged 3 commits into from
Apr 9, 2020

Conversation

sareh
Copy link
Contributor

@sareh sareh commented Apr 9, 2020

No ticket.

Currently Pashto LL date formats for the Gregorian calendar do not match our desired format.
They map to MMM DD, YYYY instead of DD MMM YYYY.

Overall change: The timestamps on Story Promos use the date format LL. This was in an incorrect format for Pashto. This PR updates LL & LLL formats.

Code changes:

  • Update LL format to DD MMM YYYY
  • Update LLL format to DD MMM YYYY HH:mm (It's good to update both of these since we may use LLL for some upcoming radio schedules work)

Testing notes - you can see the Diff in Storybook
Before (MMM DD, YYYY)
Screenshot before - in format MMM DD, YYYY
After (DD MMM YYYY)
Screenshot after - in format DD MMM YYYY

Can be observed locally in Storybook: http://localhost:8180/?path=/story/utilities-psammead-locales--moment-pashto-ps


  • I have assigned myself to this PR and the corresponding issues
  • Automated jest tests added (for new features) or updated (for existing features)
  • This PR requires manual testing

@sareh sareh added the ws-home Tasks for the WS Home Team label Apr 9, 2020
@sareh sareh self-assigned this Apr 9, 2020
Copy link
Contributor

@hotinglok hotinglok left a comment

Choose a reason for hiding this comment

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

Looks good, just forgot to change to PR number in changelog.

packages/utilities/psammead-locales/CHANGELOG.md Outdated Show resolved Hide resolved
['LL', 'فبروري ۱۴، ۲۰۱۰'],
['LLL', 'فبروري ۱۴، ۲۰۱۰ ۳:۲۵ PM'],
['LL', '۱۴ فبروري ۲۰۱۰'],
['LLL', '۱۴ فبروري ۲۰۱۰ ۱۵:۲۵'],
['D MMMM YYYY', '۱۴ فبروري ۲۰۱۰'],
],
b = moment(new Date(2010, 1, 14, 15, 25, 50, 125)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if it would be worth changing the variable names of just a and b here and in the following lines. It's understandable once I actually read into it but not at a glance. Was there any specific reason these were just called letters? If changes are to be made, it doesn't have to be in this PR. Maybe we can talk about it elsewhere first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the lines above there is a comment:
https://github.com/bbc/psammead/pull/3357/files#diff-f346ba0ea5f4f03a852d57caf66d4df3R6

// This asset overrides the gunit assertion done in the moment codebase.
// Format and styling of this file has been keep consistent with the official moment tests.
// An example of these tests can be seen at https://github.com/moment/moment/blob/develop/src/test/locale/en-gb.js

We haven't refactored the test structure, so we can contribute these suggestions back to the Moment codebase, after we believe they're complete.

Copy link
Contributor

@DenisHdz DenisHdz left a comment

Choose a reason for hiding this comment

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

Apart from @lokh01 comments, looks good to me 👍

@sareh sareh requested a review from hotinglok April 9, 2020 08:20
@sareh
Copy link
Contributor Author

sareh commented Apr 9, 2020

This has been UX approved by Laura. So we can merge it in and review fully in the Simorgh page.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ws-home Tasks for the WS Home Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants