Skip to content

Add date/time format setting in portal#16796

Merged
AllForNothing merged 6 commits intogoharbor:mainfrom
SimonAlling:add-datetime-format-setting
May 24, 2022
Merged

Add date/time format setting in portal#16796
AllForNothing merged 6 commits intogoharbor:mainfrom
SimonAlling:add-datetime-format-setting

Conversation

@SimonAlling
Copy link
Contributor

@SimonAlling SimonAlling commented May 2, 2022

Currently, the format used for rendering dates and times is derived from the language/locale selected by the user. The formats used in the en-US locale ("English" in Harbor's GUI) are ambiguous and hard to understand for many users.

For example, is 10/11/21 the 10th of November, 2021, the 11th of October, 2021, or even something else like the 21nd of November, 2010? Even if one does know how to interpret it in theory, such dates are essentially enciphered and must be mentally deciphered by the user every time, incurring unnecessary cognitive load.

Similarly, many users are used to the 24-hour clock rather than the 12-hour clock (AM/PM), and so on.

This PR adds a dropdown next to the existing language selector that lets the user choose between the default format for the current locale and the internationally standardized, unambiguous ISO 8601 format. For example, when viewing a list of resources, the ISO 8601 option makes points in time display as

2021-10-11, 13:37

instead of

10/11/21, 1:37 PM

thereby improving the user experience considerably for users not familiar with the US date/time format (or, in general, the default format for the locale they have selected).

The localized versions of the "Default" label are copied from SCANNER.DEFAULT in each locale.

Fixes #16730.
Signed-off-by: Simon Alling alling.simon@gmail.com
💡 I recommend passing --color-words='[Ll]ocale|import \{ D|formatTransformer|\);|.' to git diff or git show when reviewing this PR.

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

Currently, the format used for rendering dates and times is derived from the language/locale selected by the user. The formats used in the en-US locale ("English" in Harbor's GUI) are ambiguous and hard to understand for many users.

For example, is 10/11/21 the 10th of November, 2021, the 11th of October, 2021, or even something else like the 21nd of November, 2010? Even if one does know how to interpret it in theory, such dates are essentially enciphered and must be mentally deciphered by the user every time, incurring unnecessary cognitive load.

Similarly, many users are used to the 24-hour clock rather than the 12-hour clock (AM/PM), and so on.

This PR adds a dropdown next to the existing language selector that lets the user choose between the default format for the current locale and the internationally standardized, unambiguous ISO 8601 format. For example, when viewing a list of resources, the ISO 8601 option makes points in time display as

> 2021-10-11, 13:37

instead of

> 10/11/21, 1:37 PM

thereby improving the user experience considerably for users not familiar with the US date/time format (or, in general, the default format for the locale they have selected).

The localized versions of the "Default" label are copied from `SCANNER.DEFAULT` in each locale.

Signed-off-by: Simon Alling <alling.simon@gmail.com>
@SimonAlling SimonAlling requested a review from a team as a code owner May 2, 2022 15:46
@codecov
Copy link

codecov bot commented May 2, 2022

Codecov Report

Merging #16796 (8855b9d) into main (db45155) will decrease coverage by 4.41%.
The diff coverage is 45.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16796      +/-   ##
==========================================
- Coverage   71.62%   67.20%   -4.42%     
==========================================
  Files         736      966     +230     
  Lines       67491    80167   +12676     
  Branches        0     2550    +2550     
==========================================
+ Hits        48342    53879    +5537     
- Misses      15769    22616    +6847     
- Partials     3380     3672     +292     
Flag Coverage Δ
unittests 67.20% <45.71%> (-4.42%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ortal/src/app/shared/pipes/harbor-datetime.pipe.ts 36.00% <26.31%> (ø)
...shared/components/navigator/navigator.component.ts 42.85% <57.14%> (ø)
src/portal/src/app/shared/units/shared.utils.ts 36.04% <66.66%> (ø)
src/portal/src/app/shared/entities/shared.const.ts 100.00% <100.00%> (ø)
src/lib/cache/helper.go 73.91% <0.00%> (-8.70%) ⬇️
...e-integration/tag-feature-integration.component.ts 100.00% <0.00%> (ø)
.../app/base/left-side-nav/labels/labels.component.ts 100.00% <0.00%> (ø)
.../components/about-dialog/about-dialog.component.ts 93.33% <0.00%> (ø)
...etail/chart-detail/chart-detail-value.component.ts 43.75% <0.00%> (ø)
.../projects/statictics/statistics-panel.component.ts 86.36% <0.00%> (ø)
... and 227 more

@SimonAlling
Copy link
Contributor Author

Technically, 2021-10-11, 13:37 is not ISO 8601-compliant (because ISO 8601 doesn't allow any other separator than T between the date and the time). However, 2021-10-11 and 13:37 are compliant date and time expressions, respectively. Not sure if the option should have a more accurate name or what that would be …

Signed-off-by: Simon Alling <alling.simon@gmail.com>
@SimonAlling SimonAlling force-pushed the add-datetime-format-setting branch from 547265d to 457ec17 Compare May 3, 2022 08:57
@SimonAlling
Copy link
Contributor Author

Why do I have to sign off commits I push to the PR branch after the initial one? The PR should be squash-merged anyway.

@AllForNothing AllForNothing added release-note/enhancement Label to mark PR to be added under release notes as enhancement release-note/docs Docs changes(made and needed) labels May 5, 2022
@AllForNothing
Copy link
Contributor

@SimonAlling Please rebase your code to resolve conflicts and squash the two commits into one

@AllForNothing
Copy link
Contributor

Why do I have to sign off commits I push to the PR branch after the initial one? The PR should be squash-merged anyway.

Every commit should be signed off. And one PR should only contain one commit

@SimonAlling SimonAlling force-pushed the add-datetime-format-setting branch from 2473e30 to 4193fa2 Compare May 5, 2022 08:55
@AllForNothing
Copy link
Contributor

@SimonAlling Please run git rebase -i origin/main, then use squash to squash your commits into one, now you have
4 commits

@SimonAlling
Copy link
Contributor Author

SimonAlling commented May 5, 2022

@SimonAlling Please rebase your code to resolve conflicts and squash the two commits into one

I have resolved the merge conflicts. 👍

I'd rather not rewrite the history of this PR branch though, unless there is a really good reason. Please see #16810.

And one PR should only contain one commit

Squash-merging can be used to create a single commit on the trunk without rewriting the history of the PR branch (details in #16810). Here is my suggested commit message body:

Details
Currently, the format used for rendering dates and times is derived from the language/locale selected by the user. The formats used in the en-US locale ("English" in Harbor's GUI) are ambiguous and hard to understand for many users.

For example, is 10/11/21 the 10th of November, 2021, the 11th of October, 2021, or even something else like the 21nd of November, 2010? Even if one does know how to interpret it in theory, such dates are essentially enciphered and must be mentally deciphered by the user every time, incurring unnecessary cognitive load.

Similarly, many users are used to the 24-hour clock rather than the 12-hour clock (AM/PM), and so on.

This PR adds a dropdown next to the existing language selector that lets the user choose between the default format for the current locale and the internationally standardized, unambiguous ISO 8601 format. For example, when viewing a list of resources, the ISO 8601 option makes points in time display as

> 2021-10-11, 13:37

instead of

> 10/11/21, 1:37 PM

thereby improving the user experience considerably for users not familiar with the US date/time format (or, in general, the default format for the locale they have selected).

The localized versions of the "Default" label are copied from `SCANNER.DEFAULT` in each locale.

Fixes #16730.

💡 `git show --color-words='[Ll]ocale|import \{ D|formatTransformer|\);|.'` is useful for viewing this commit.

@AllForNothing
Copy link
Contributor

@SimonAlling We had a discussion about your PR, we will merge this RP after your rebasing code and squashing commits into one

@SimonAlling
Copy link
Contributor Author

@SimonAlling We had a discussion about your PR, we will merge this RP after your rebasing code and squashing commits into one

Could you please share your conclusions and/or elaborate on why squash-merging doesn't suffice, either here or in #16810? My suggested commit message body can be copied from an earlier comment and, unlike rewriting the history of the PR branch, squash-merging guarantees a single, non-merge commit on the trunk. How are you guys planning on merging the PR anyway, if not by squash-merging?

In my opinion, rebasing and force-pushing PR branches left and right goes completely against the spirit of Git, nullifies many of the benefits of using Git and cannot be justified without good reason (e.g. signing off retroactively). I'd be happy to learn about other ways to see things though!

@AllForNothing
Copy link
Contributor

@SimonAlling In my opinion, the one commit per PR rule can be understood as one PR accomplishing one goal with one commit. If one commit is not enough to accomplish this goal, we can use multiple PRs to accomplish this goal, rather than one PR containing multiple commits. This can also avoid a single PR being too large.

In actual development, in order to achieve a certain goal, we may decompose it into many steps, and each step may be a commit, but this does not mean that every commit is what we care about. So we use the git commit --amend command to merge those commits information, leaving only one commit, which is usually enough to explain what this PR has done.

Take your PR as an example:
image

@SimonAlling
Copy link
Contributor Author

@AllForNothing, it seems like we have the exact same opinion and goal – great! 😃 The easiest and best way to accomplish that goal is to select Squash and merge and then paste my suggested commit message body from earlier in the thread (or edit GitHub's generated body manually).

Squash and merge

That will create a single, non-merge commit on the trunk, just like if I would squash and rebase this branch and it would then be rebase-merged. However, squash-merging has these advantages:

  • People can collaborate on PRs.
  • Maintainers can contribute to PRs (if enabled by the PR author).
  • Reviewers can see how a PR has changed since they last reviewed it.
  • Contributors need not go through the hassle of rebasing and force-pushing.
  • Reviewers need not request that contributors rebase and force-push.
  • Force-pushing can be reserved for when it's actually needed.
  • The end result is guaranteed to be a single, non-merge commit on the trunk. (Even a PR with only a single commit can be merged as a merge commit – this happened to PRs 16886, 16833, 16807, etc …)

Unfortunately, GitHub generates a cluttered commit message body by default, so care must be taken when performing the merge. (This is already the case today: maintainers apparently often accidentally merge PRs as merge commits.)

(I think this discussion belongs in #16810, but it hasn't seen too much activity.)

@AllForNothing
Copy link
Contributor

@SimonAlling Can you resolve the conflicts, then I will use Squash and merge option to merge this PR

@SimonAlling
Copy link
Contributor Author

@AllForNothing, cool! I have resolved the conflicts and auto-formatted the code.

I noticed when I ran the test suite that hundreds of these warnings are spammed in the output:

WARN: 'Invalid saved datetime rendering setting null; defaulting to "locale-default".'

So I assume the test suite should be tweaked in some way so that there is a valid saved value when running tests.

@AllForNothing AllForNothing self-requested a review May 24, 2022 09:44
Copy link
Contributor

@AllForNothing AllForNothing left a comment

Choose a reason for hiding this comment

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

LGTM

@AllForNothing AllForNothing merged commit c4b782b into goharbor:main May 24, 2022
@AllForNothing
Copy link
Contributor

Will raise a subsequent PR to clear the warnings

@SimonAlling
Copy link
Contributor Author

Cool!

As a piece of advice for future PRs, note that extra care needs to be taken when squash-merging, because GitHub's suggested message surprisingly and annoyingly consists of the messages of all commits on the PR branch (e.g. Fix indentation and Run 'npm run lint -- --fix'), instead of the PR description, which is almost always what one wants in the final commit message.

@SimonAlling SimonAlling deleted the add-datetime-format-setting branch May 24, 2022 10:22
YangJiao0817 pushed a commit to YangJiao0817/harbor that referenced this pull request May 25, 2022
Update it because goharbor#16796 modifies head_admin_xpath.

Signed-off-by: Yang Jiao <jiaoya@vmware.com>
YangJiao0817 pushed a commit to YangJiao0817/harbor that referenced this pull request May 26, 2022
Update it because goharbor#16796 modifies head_admin_xpath.

Signed-off-by: Yang Jiao <jiaoya@vmware.com>
YangJiao0817 pushed a commit to YangJiao0817/harbor that referenced this pull request May 26, 2022
Update it because goharbor#16796 modifies head_admin_xpath.

Signed-off-by: Yang Jiao <jiaoya@vmware.com>
YangJiao0817 pushed a commit to YangJiao0817/harbor that referenced this pull request May 27, 2022
Update it because goharbor#16796 modifies head_admin_xpath.

Signed-off-by: Yang Jiao <jiaoya@vmware.com>
YangJiao0817 pushed a commit to YangJiao0817/harbor that referenced this pull request May 27, 2022
Update it because goharbor#16796 modifies header user.

Signed-off-by: Yang Jiao <jiaoya@vmware.com>
@YangJiao0817 YangJiao0817 mentioned this pull request May 27, 2022
5 tasks
sluetze pushed a commit to sluetze/harbor that referenced this pull request Oct 29, 2022
* Add date/time format setting in portal

Currently, the format used for rendering dates and times is derived from the language/locale selected by the user. The formats used in the en-US locale ("English" in Harbor's GUI) are ambiguous and hard to understand for many users.

For example, is 10/11/21 the 10th of November, 2021, the 11th of October, 2021, or even something else like the 21nd of November, 2010? Even if one does know how to interpret it in theory, such dates are essentially enciphered and must be mentally deciphered by the user every time, incurring unnecessary cognitive load.

Similarly, many users are used to the 24-hour clock rather than the 12-hour clock (AM/PM), and so on.

This PR adds a dropdown next to the existing language selector that lets the user choose between the default format for the current locale and the internationally standardized, unambiguous ISO 8601 format. For example, when viewing a list of resources, the ISO 8601 option makes points in time display as

> 2021-10-11, 13:37

instead of

> 10/11/21, 1:37 PM

thereby improving the user experience considerably for users not familiar with the US date/time format (or, in general, the default format for the locale they have selected).

The localized versions of the "Default" label are copied from `SCANNER.DEFAULT` in each locale.

Signed-off-by: Simon Alling <alling.simon@gmail.com>

* Fix indentation

Signed-off-by: Simon Alling <alling.simon@gmail.com>

* Remove redundant localStorage existence check

Signed-off-by: Simon Alling <alling.simon@gmail.com>

* Run 'npm run lint -- --fix'
sluetze pushed a commit to sluetze/harbor that referenced this pull request Oct 29, 2022
Update it because goharbor#16796 modifies header user.

Signed-off-by: Yang Jiao <jiaoya@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/docs Docs changes(made and needed) release-note/enhancement Label to mark PR to be added under release notes as enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make time format configurable

3 participants