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 Tab view on settings page #3551

Merged
merged 14 commits into from
Aug 13, 2021

Conversation

gautamjajoo
Copy link
Member

@gautamjajoo gautamjajoo commented Jul 26, 2021

This PR:

  • restructures the settings page with tab view divided into three tabs.
  • makes phase splits private when phase is made public
  • added a check when phase splits made public when phase is private

@Ram81 @Kajol-Kumari

tab-view.mp4

Copy link
Member

@Kajol-Kumari Kajol-Kumari left a comment

Choose a reason for hiding this comment

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

@gautamjajoo minor change required,
rest LGTM 👍

@gautamjajoo
Copy link
Member Author

@Kajol-Kumari Done!

@gautamjajoo
Copy link
Member Author

mat-tab-label styles were channging when coming from public-list component. Fixed it by adding :host for child component.

@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2021

Codecov Report

Merging #3551 (8558f63) into master (96968d6) will decrease coverage by 1.49%.
The diff coverage is 32.43%.

@@            Coverage Diff             @@
##           master    #3551      +/-   ##
==========================================
- Coverage   72.93%   71.44%   -1.50%     
==========================================
  Files          83       20      -63     
  Lines        5368     3232    -2136     
==========================================
- Hits         3915     2309    -1606     
+ Misses       1453      923     -530     
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.41% <30.64%> (-10.29%) ⬇️
frontend/src/js/controllers/updateProfileCtrl.js 82.55% <44.44%> (-10.30%) ⬇️
frontend/src/js/controllers/challengeListCtrl.js 94.68% <50.00%> (ø)
...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 31 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.41% <30.64%> (-10.29%) ⬇️
frontend/src/js/controllers/updateProfileCtrl.js 82.55% <44.44%> (-10.30%) ⬇️
frontend/src/js/controllers/challengeListCtrl.js 94.68% <50.00%> (ø)
...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 31 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 c509e38...8558f63. Read the comment docs.

Copy link
Member

@Kajol-Kumari Kajol-Kumari 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 we please fix the below minor responsiveness issues -

  1. The private tag for phase select box shouldn't get hidden for small screen. ref ⬇️

Screenshot 2021-08-04 at 9 29 20 PM

  1. The left-padding for Evaluation tab goes off for screens smaller than 700px. We should keep it consistent throughout. ref ⬇️

Screenshot 2021-08-04 at 9 29 56 PM

@gautamjajoo
Copy link
Member Author

gautamjajoo commented Aug 4, 2021

@Kajol-Kumari What was the screen size for the phase selected box?. Got it!

@gautamjajoo
Copy link
Member Author

@Kajol-Kumari Updated the PR!

Copy link
Member

@Kajol-Kumari Kajol-Kumari left a comment

Choose a reason for hiding this comment

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

@gautamjajoo The padding on the settings page doesn't look consistent with the other pages, please fix it. See the ref image below -
Screenshot 2021-08-06 at 4 32 42 PM

Screenshot 2021-08-06 at 4 32 34 PM

@@ -28,6 +28,17 @@
padding-right: 0px;
}

@media (max-width: 400px) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep it till 800px as the private-tag issue is for ipad sized screens as well

@gautamjajoo
Copy link
Member Author

gautamjajoo commented Aug 6, 2021

@Kajol-Kumari In the above screenshot is it regarding the space between title and the tabs? Because it was intentional as they were tabs so I thought some space would be needed?

@Kajol-Kumari
Copy link
Member

Kajol-Kumari commented Aug 6, 2021

@Kajol-Kumari In the above screenshot is it regarding the space between title and the tabs? Because it was intentional as they were tabs so I thought some space would be needed?

I am taking about the top-padding for the title. Here the top-padding is larges as compared with the other tabs. Also the spacing btw title and tabs can also be reduced a bit.(we can keep it same as the top padding but the top padding should be same as other tabs)

@gautamjajoo
Copy link
Member Author

@Kajol-Kumari Done the changes!

Copy link
Member

@Kajol-Kumari Kajol-Kumari left a comment

Choose a reason for hiding this comment

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

Thanks for making the requested changes @gautamjajoo
LGTM 👍

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.

Screen Shot 2021-08-08 at 7 00 06 PM

@gautamjajoo can we add heading Evaluation script on he Evaluation tab

@gautamjajoo
Copy link
Member Author

@Ram81 Heading of the first card where evaluation script and criteria is present?

@Ram81
Copy link
Member

Ram81 commented Aug 9, 2021

Yes

@gautamjajoo
Copy link
Member Author

Should we add like this or the one similar to edit phase and leaderboard?
Screenshot from 2021-08-09 13-46-13

@RishabhJain2018 RishabhJain2018 merged commit c9b05b5 into Cloud-CV:master Aug 13, 2021
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.

None yet

5 participants