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(openchallenges): challenge card fixes #2315

Merged
merged 5 commits into from
Nov 3, 2023

Conversation

vpchung
Copy link
Member

@vpchung vpchung commented Nov 3, 2023

Changelog

  • add a new parameter to calcTimeDiff, so that hiding timeline > 3 months is optional

    • use case: for active and upcoming challenges, it is still valuable to show the temporal info, even if it's beyond 3 months away
  • add type annotation for return value (feedback)

  • if an unexpected date format is given, throw an Error instead of returning an empty string (feedback)

    • relatedly, add a try-catch for function call

    [!NOTE]
    I had tried the other approach mentioned in the feedback, but I kept getting an error about not being able to do the arithmetic operation when approaching things that way

Preview

Before
Screenshot 2023-11-02 at 4 58 30 PM

After
Screenshot 2023-11-02 at 4 58 42 PM

@vpchung vpchung marked this pull request as ready for review November 3, 2023 01:21
Copy link
Member

@tschaffter tschaffter left a comment

Choose a reason for hiding this comment

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

I've made a few suggestions, otherwise it's good to merge.

@@ -92,7 +98,7 @@ export class ChallengeCardComponent implements OnInit {
// Find the largest unit of time and return in human-readable format.
let timeDiffString = '';
for (const [unit, value] of Object.entries(timeDiff)) {
if (unit === 'month' && value > 3) {
if (hideFarDates && unit === 'month' && value > 3) {
Copy link
Member

Choose a reason for hiding this comment

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

Far away dates?

Suggested change
if (hideFarDates && unit === 'month' && value > 3) {
if (hideFarAwayDates && unit === 'month' && value > 3) {

@vpchung vpchung merged commit 2915ab6 into Sage-Bionetworks:main Nov 3, 2023
6 checks passed
@vpchung vpchung deleted the challenge-card-fix branch November 3, 2023 17:51
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.

2 participants