-
Notifications
You must be signed in to change notification settings - Fork 61
Conversation
modified the challenge leadeboard page, changed the url by appending the phase split id and by default setted the top phase split id in the url
@Sanji515 Please resolve the conflicts here. |
Codecov Report
@@ Coverage Diff @@
## master #155 +/- ##
==========================================
- Coverage 52.3% 52.14% -0.17%
==========================================
Files 63 63
Lines 3361 3404 +43
Branches 383 384 +1
==========================================
+ Hits 1758 1775 +17
- Misses 1510 1535 +25
- Partials 93 94 +1
Continue to review full report at Codecov.
|
src/app/components/challenge/challengeleaderboard/challengeleaderboard.component.html
Outdated
Show resolved
Hide resolved
src/app/components/challenge/challengeleaderboard/challengeleaderboard.component.ts
Show resolved
Hide resolved
src/app/components/challenge/challengeleaderboard/challengeleaderboard.component.ts
Outdated
Show resolved
Hide resolved
src/app/components/challenge/challengesubmit/challengesubmit.component.html
Show resolved
Hide resolved
src/app/components/challenge/challengesubmit/challengesubmit.component.html
Show resolved
Hide resolved
src/app/components/challenge/challengesubmit/challengesubmit.component.html
Show resolved
Hide resolved
@RishabhJain2018 it works fine except when we click on |
I've opened the PR for the worker fix. @Sanji515 Please take a look at that and then test it. |
@RishabhJain2018 @lunayach Resolved all the comments and tested the functionality, it's working perfectly on my side. Please review 😃 |
Hi @Sanji515, I was referring to this issue. When I click on the update button, the |
@Sanji515 The issue mentioned by Mayank is fixed, right? |
if (SELF.selectedPhaseSplit['phase_split']) { | ||
SELF.fetchLeaderboard(SELF.selectedPhaseSplit['phase_split']['id']); | ||
} | ||
SELF.router.navigate([phaseSplit['id']], {relativeTo: this.route}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, thanks 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to be merged now.
Two points:
- Currently, the "Page is Outdated, Click to update" button appears only when the no. of submissions for that particular phase and split has changed. But what if the host re-runs the submission of the user(s). I think an additional check for this should also be included.
- The toggle button, Show on Leaderboard does not seem to be rendering properly.
@Sanji515 @RishabhJain2018 should we create issues to take care of the above points later. (IDK, maybe the second point could be addressed in this PR itself.)
@Sanji515 Can you please take care of both the things in a separate PR? |
Changes proposed in this pull request:
Modified the challenge leadeboard page, changed the url by appending the phase split id and by default setted the top phase split id in the url
Link to live demo: http://pr-154-evalai.surge.sh
Screenshot 1:
Screenshot 2: