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.

Sign up for GitHub

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

Jump to bottom

GIX-1872 Make secondsToDuration consistent with daysToDuration #3310

Merged
merged 4 commits into from
Sep 13, 2023
Merged

GIX-1872 Make secondsToDuration consistent with daysToDuration #3310

merged 4 commits into from
Sep 13, 2023

Conversation

dskloetd
Copy link
Contributor

@dskloetd dskloetd commented Sep 12, 2023 •
edited
Loading

Motivation

We use both secondsToDuration and daysToDuration to render the same time period so they should be consistent but currently they are not.

This PR fixes https://dfinity.atlassian.net/browse/GIX-1872.
It does not aim to fix https://dfinity.atlassian.net/browse/GIX-1770 which is still being discussed.
I think it's worthwhile to fix GIX-1872 and put a test in place that the rendering is consistent before we have a decision on GIX-1770.

Changes

  1. Reimplement secondsToDuration.
  2. Update tests for secondsToDuration for the new implementation.
  3. Test that both functions return the same string for periods that are a whole number of days between 1 and 3000.

Tests

Extensive unit test are updated.

Todos

  • Add entry to changelog (if necessary).

@dskloetd dskloetd changed the title Make secondsToDuration consistent with daysToDuration GIX-1872 Make secondsToDuration consistent with daysToDuration Sep 12, 2023
@dskloetd dskloetd marked this pull request as ready for review September 12, 2023 17:45
@dskloetd dskloetd requested a review from lmuntaner September 12, 2023 17:45
lmuntaner
lmuntaner reviewed Sep 12, 2023
View reviewed changes
Copy link
Contributor

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

Why didn't you use the daysToDuration within secondsToDuration?

Don't we want that?

frontend/src/lib/utils/date.utils.ts Show resolved Hide resolved
@dskloetd dskloetd enabled auto-merge September 13, 2023 06:37
@dskloetd dskloetd added this pull request to the merge queue Sep 13, 2023
Merged via the queue into main with commit 6025503 Sep 13, 2023
38 checks passed
@dskloetd dskloetd deleted the kloet/fix-duration branch September 13, 2023 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmuntaner lmuntaner lmuntaner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dskloetd @lmuntaner