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 challenge phase detail edit feature for more fields #3491

Merged
merged 13 commits into from
Jul 11, 2021

Conversation

gautamjajoo
Copy link
Member

@gautamjajoo gautamjajoo commented Jun 20, 2021

Add challenge phase detail edit feature for following fields:

  • allowed submission file types
  • maximum concurrent submissions
  • phase visibility
  • submission visibility
  • default meta attributes

@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2021

Codecov Report

Merging #3491 (82d28e3) into master (96968d6) will decrease coverage by 0.82%.
The diff coverage is 34.17%.

@@            Coverage Diff             @@
##           master    #3491      +/-   ##
==========================================
- Coverage   72.93%   72.10%   -0.83%     
==========================================
  Files          83       20      -63     
  Lines        5368     3194    -2174     
==========================================
- Hits         3915     2303    -1612     
+ Misses       1453      891     -562     
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 64.55% <32.84%> (-9.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 64.55% <32.84%> (-9.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 9d0e043...82d28e3. 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 The phase and submission visiblity change makes the modal to close adrupty ignoring the rest of the changes. Instead, we can make it either -

  1. The visibility should change only after we click on submit button of the edit phase modal
    or
  2. After we select the phase, we can show three option -
    a. Edit Detail - It will make the edit phase modal allowing users to edit phase details except submission and phase visibility.
    b. Phase Visibility - It will allow to change only the phase visibility
    c. Submission Visibility - It will allow users to change only the submission visibility

@@ -76,6 +100,7 @@
</div>
</div>

<div class="row row-lr-margin">
Copy link
Member

Choose a reason for hiding this comment

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

Formatting seems a bit off in this file, please fix it.

@@ -135,6 +160,43 @@
</div>
</div>

<div class="col-lg-4 col-md-4 col-sm-12 col-lr-pad">
Copy link
Member

Choose a reason for hiding this comment

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

please keep this div outside the submissions div i.e after [this] line. Moreover make a separate div and add the Max Concurrent Submissions Allowed and Allowed Submission File Types in a single row of class col-lg-6 col-md-6 col-sm-6 col-lr-pad each

@@ -1,6 +1,12 @@
import { ViewChildren, QueryList, Component, Input, OnInit } from '@angular/core';
import { GlobalService } from '../../../../services/global.service';
import { InputComponent } from '../../../utility/input/input.component';
import { NGXLogger } from 'ngx-logger';

Copy link
Member

Choose a reason for hiding this comment

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

please remove this extra line gap

@@ -113,6 +149,22 @@ export class EditphasemodalComponent implements OnInit {
*/
editPhaseDetails = true;

/**
* publish challenge state and it's icon
Copy link
Member

Choose a reason for hiding this comment

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

it should be phase visibility state and it's icon instead of publish challenge state and it's icon

};

/**
* publish challenge state and it's icon
Copy link
Member

Choose a reason for hiding this comment

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

it should be submission visibility state and it's icon here.

@@ -188,6 +269,124 @@ export class EditphasemodalComponent implements OnInit {
this.todayDateTime = new Date();
}

/**
* Phase Visibility click function
Copy link
Member

Choose a reason for hiding this comment

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

it should be phase visibility toggle function

}

/**
* Submission Visibility click function
Copy link
Member

Choose a reason for hiding this comment

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

please change it to Submission Visibility toggle function

@gautamjajoo
Copy link
Member Author

gautamjajoo commented Jun 28, 2021

@Kajol-Kumari Done the required changes!
Screenshot from 2021-06-28 21-30-29

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 condition for the the edit, phase visibiiity change and submission visibility change needs to be modified as after we change the visibility, the selected phase value becomes undefined but still those buttons are visible.

Ref -

Screen.Recording.2021-06-29.at.9.07.03.PM.mov

class="btn ev-btn-dark waves-effect waves-dark grad-btn grad-btn-dark fs-12"
(click)="togglePhaseVisibility()"
>
<i class="{{ phaseVisibility.icon }}"></i> Phase Visibility
Copy link
Member

Choose a reason for hiding this comment

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

We need to bind the icon with the selected phase's phase visibility otherwise, it will show the ambiguous behaviour. Please see the recording below to see the ambiguity -

Screen.Recording.2021-06-29.at.8.58.45.PM.mov

class="btn ev-btn-dark waves-effect waves-dark grad-btn grad-btn-dark fs-12"
(click)="toggleSubmissionVisibility()"
>
<i class="{{ submissionVisibility.icon }}"></i> Submission Visibility
Copy link
Member

Choose a reason for hiding this comment

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

Same here! Please bind it with the selected phases's submission visibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe we don't need to bind it to the particular phase, it's just that I missed the else statement.

<div class="row row-lr-margin">
<div class="col-lg-6 col-md-6 col-sm-6 col-lr-pad">
<div class="input-field align-left">
<div class="fs-16 fw-regular text-med-black">Allowed Submission File Types</div>
Copy link
Member

Choose a reason for hiding this comment

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

Can we please keep Max Concurrent Submissions Allowed and Allowed Submission File Types in the same row as there is a lot of empty space on the right side. Please see the ss below -
Screenshot 2021-06-29 at 9 04 22 PM

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!

@@ -2,6 +2,7 @@ import { ViewChildren, QueryList, Component, Input, OnInit } from '@angular/core
import { GlobalService } from '../../../../services/global.service';
import { InputComponent } from '../../../utility/input/input.component';


Copy link
Member

Choose a reason for hiding this comment

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

please revert this change

@gautamjajoo
Copy link
Member Author

@gautamjajoo The condition for the the edit, phase visibiiity change and submission visibility change needs to be modified as after we change the visibility, the selected phase value becomes undefined but still those buttons are visible.

@Kajol-Kumari This error is encountered because we are fetching the phase details again after they are updated. I will look for a different method from which we can update the details.

@gautamjajoo
Copy link
Member Author

@Kajol-Kumari @Ram81 I have updated the PR, fixing the above issues.

The error which was mentioned that the phase are not visible on the drop-down menu after phase details are updated, was becuase we were using hide-label when any phaseName was not selected. But after the details are updated, ideally the phaseName variable retained the previous phase name value. Hence, using the challenge.service whenever the phase details are updated we change the phaseName to '', which makes the label work perfectly.

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.

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.

@gautamjajoo the Phase details button after selecting the phase should also show a edit icon. It's not clear what the Phase details button is for. Also the Phase visibility and Submission visibility toggle button can go in the Phase details pop up or they should look like toggle buttons.

@gautamjajoo
Copy link
Member Author

@Ram81 Is this fine?
Screenshot from 2021-07-08 16-59-35

@Ram81
Copy link
Member

Ram81 commented Jul 10, 2021

@gautamjajoo can we move the toggle bottons slightly down. They are misaligned from the labels. Also, rename Phase Visibility to Is Public

@Ram81
Copy link
Member

Ram81 commented Jul 11, 2021

@gautamjajoo can you share the updated screenshot?

@gautamjajoo
Copy link
Member Author

gautamjajoo commented Jul 11, 2021

@Ram81 The toggle buttons are now slightly down
Screenshot from 2021-07-11 15-03-43

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.

LGTM

@Ram81 Ram81 merged commit 2bc8762 into Cloud-CV:master Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants