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

Add edit option for submission guidelines and leaderboard description #3569

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gautamjajoo
Copy link
Member

@codecov-commenter
Copy link

codecov-commenter commented Aug 14, 2021

Codecov Report

Merging #3569 (16e5b2d) into master (96968d6) will decrease coverage by 1.40%.
The diff coverage is 33.39%.

@@            Coverage Diff             @@
##           master    #3569      +/-   ##
==========================================
- Coverage   72.93%   71.53%   -1.41%     
==========================================
  Files          83       20      -63     
  Lines        5368     3235    -2133     
==========================================
- Hits         3915     2314    -1601     
+ Misses       1453      921     -532     
Impacted Files Coverage Δ
frontend/src/js/controllers/authCtrl.js 53.91% <6.38%> (-12.95%) ⬇️
frontend/src/js/controllers/profileCtrl.js 79.76% <20.00%> (-13.10%) ⬇️
frontend/src/js/controllers/permissionCtrl.js 36.36% <22.22%> (-63.64%) ⬇️
frontend/src/js/controllers/challengeCtrl.js 63.54% <31.97%> (-10.15%) ⬇️
frontend/src/js/controllers/updateProfileCtrl.js 82.55% <44.44%> (-10.30%) ⬇️
frontend/src/js/controllers/challengeListCtrl.js 95.74% <50.00%> (+1.06%) ⬆️
...ntend/src/js/controllers/challengeHostTeamsCtrl.js 70.50% <66.66%> (-1.18%) ⬇️
frontend/src/js/controllers/teamsCtrl.js 71.17% <75.00%> (ø)
frontend/src/js/controllers/ChallengeInviteCtrl.js 100.00% <100.00%> (ø)
frontend/src/js/controllers/SubmissionFilesCtrl.js 95.45% <100.00%> (ø)
... and 30 more
Impacted Files Coverage Δ
frontend/src/js/controllers/authCtrl.js 53.91% <6.38%> (-12.95%) ⬇️
frontend/src/js/controllers/profileCtrl.js 79.76% <20.00%> (-13.10%) ⬇️
frontend/src/js/controllers/permissionCtrl.js 36.36% <22.22%> (-63.64%) ⬇️
frontend/src/js/controllers/challengeCtrl.js 63.54% <31.97%> (-10.15%) ⬇️
frontend/src/js/controllers/updateProfileCtrl.js 82.55% <44.44%> (-10.30%) ⬇️
frontend/src/js/controllers/challengeListCtrl.js 95.74% <50.00%> (+1.06%) ⬆️
...ntend/src/js/controllers/challengeHostTeamsCtrl.js 70.50% <66.66%> (-1.18%) ⬇️
frontend/src/js/controllers/teamsCtrl.js 71.17% <75.00%> (ø)
frontend/src/js/controllers/ChallengeInviteCtrl.js 100.00% <100.00%> (ø)
frontend/src/js/controllers/SubmissionFilesCtrl.js 95.45% <100.00%> (ø)
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c37549...16e5b2d. Read the comment docs.

Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

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

@gautamjajoo edit options for Submission guidelines and Leaderboard Description should be separate. We shouldn't show it as part of the phase or phase split edit dropdown options

@gautamjajoo
Copy link
Member Author

We would have a separate section for this right? What should be the title of that seciton?

@Kajol-Kumari
Copy link
Member

Kajol-Kumari commented Aug 23, 2021

We would have a separate section for this right? What should be the title of that section?

@gautamjajoo Instead of making a whole new section can we please keep these in a new row. Like we can keep Submission Guideline edit option in parallel with test annotation upload done in this PR

And we can keep Leaderboard Description edit option in a new row below the Leaderboard Visibility row

\cc: @Ram81 please share your thoughts on the same

@gautamjajoo
Copy link
Member Author

@Kajol-Kumari Actually we thought of buttons becuase initially it was done the way you have mentioned but it was felt that it is a part of phase selection that's why we changed it to buttons.

Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

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

@gautamjajoo can you share the screenshot of the page and resolve merge conflicts

@gautamjajoo
Copy link
Member Author

Resolved the conflicts and here is the screenshot.
Screenshot from 2021-08-20 22-51-09

@RishabhJain2018
Copy link
Member

@Ram81 Can you please review this once more?

Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

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

Minor comments, rest of the PR LGTM

.subscribe(
(data) => {
SELF.challenge.submission_guidelines = data.submission_guidelines;
SELF.globalService.showToast('success', 'The submission guidelines are successfully updated!', 5);
Copy link
Member

Choose a reason for hiding this comment

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

@gautamjajoo please change the message to Submission guidelines updated successfully!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

.subscribe(
(data) => {
SELF.challenge.leaderboard_description = data.leaderboard_description;
SELF.globalService.showToast('success', 'The leaderboard description is successfully updated!', 5);
Copy link
Member

Choose a reason for hiding this comment

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

@gautamjajoo please change the message to Leaderboard description updated successfully!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

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

@gautamjajoo can resolve the merge conflcts. This PR is ready to be merged now, we can merge it once you resolve the conflicts 🙂

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.

5 participants