Assessment Configurability in Ground Control #2863
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nable tokencounter depending on assessment hasTokenCounter, not assessmentConfig
…be true to backend
Pull Request Test Coverage Report for Build 8438080400Details
💛 - Coveralls |
5 tasks
|
Corresponding backend PR: source-academy/backend#1093 |
The stronger type checking had been causing compile errors.
…-academy-frontend into EnhancedGroundControl
|
@joeng03 @martin-henz if you get to reviewing this before I do, please don't immediately merge it upon approval as I expect this to be a 3-way merge conflict with 2 other PRs. |
…-academy-frontend into EnhancedGroundControl
RichDom2185
reviewed
Mar 26, 2024
There was a problem hiding this comment.
Hi, thanks for working on this, looking good! I just have some minor comments below.
As discussed, the order of merging will be:
- PR Add Team Assessments #2548
- PR Clean up team assessments #2875
- This PR
Comment on lines
41
to
52
| const toggleHasTokenCounter = useCallback( | ||
| () => setHasTokenCounter(!hasTokenCounter), | ||
| [hasTokenCounter] | ||
| ); | ||
| const toggleVotingFeatures = useCallback( | ||
| () => setHasVotingFeatures(!hasVotingFeatures), | ||
| [hasVotingFeatures] | ||
| ); | ||
| const toggleIsTeamAssessment = useCallback( | ||
| () => setIsTeamAssessment(!isTeamAssessment), | ||
| [isTeamAssessment] | ||
| ); |
There was a problem hiding this comment.
Nit: Here, we memoize, but we recreate every time isXXXX changes; this is not very ideal.
When toggling booleans, if we use the callback notation for setXXX, we can simply create the function once and not have to recreate it again:
Suggested change
| const toggleHasTokenCounter = useCallback( | |
| () => setHasTokenCounter(!hasTokenCounter), | |
| [hasTokenCounter] | |
| ); | |
| const toggleVotingFeatures = useCallback( | |
| () => setHasVotingFeatures(!hasVotingFeatures), | |
| [hasVotingFeatures] | |
| ); | |
| const toggleIsTeamAssessment = useCallback( | |
| () => setIsTeamAssessment(!isTeamAssessment), | |
| [isTeamAssessment] | |
| ); | |
| const toggleHasTokenCounter = useCallback( | |
| () => setHasTokenCounter(prev => !prev), | |
| [] | |
| ); | |
| const toggleVotingFeatures = useCallback( | |
| () => setHasVotingFeatures(prev => !prev), | |
| [] | |
| ); | |
| const toggleIsTeamAssessment = useCallback( | |
| () => setIsTeamAssessment(prev => !prev), | |
| [] | |
| ); |
| <p> | ||
| <b>General Configurations</b> | ||
| </p> | ||
| <Divider></Divider> |
There was a problem hiding this comment.
Nit:
Suggested change
| <Divider></Divider> | |
| <Divider /> |
There was a problem hiding this comment.
Ditto for the other occurrences in this file.
| onChange={toggleHasTokenCounter} | ||
| inline | ||
| label="Enable token counter" | ||
| ></Switch> |
There was a problem hiding this comment.
Nit:
Suggested change
| ></Switch> | |
| /> |
| </DialogBody> | ||
| <DialogFooter | ||
| actions={ | ||
| <> |
…-academy-frontend into EnhancedGroundControl
|
I've polished up the code to fix the above comments. Pls let me know if there are any further issues! |
|
Will just wait for backend PR to pass review now before merging both at the same time. |
izruff
pushed a commit
that referenced
this pull request
Apr 2, 2024
* Change hasTokenCounter to hasVotingAttributes in Assessment Config. Enable tokencounter depending on assessment hasTokenCounter, not assessmentConfig * Rename hasVotingAttributes to hasVotingFeatures * Create dialog box for configuration in ground control * Create dialog box for configurations with voting config toggles * Combine toggleHasTokenCounter and toggleVotingFeatures into configureAssessment * Create backend request for updating configurations * Add message for updating assessment configurations * Isolate hastokencounter as its own field * Add non-functional voting controls * Fix tests * Add new column for token counter in admin panel * Fix bugs relating to tokencounter switch * Remove unnecessary maxwidth property on tokencounter switch * Fix bug where toggling tokencounter would request voting features to be true to backend * Create footer text to clarify usage of hastokencounter and hasVotingFeatures * Align text on configure dialogbox * Fix tests post-merge The stronger type checking had been causing compile errors. * Fix imports * Add close dialog upon saving * Uncapitalise letters in config dialog * Change label from has token counter to enable token counter * Code quality improvements * Create new section for team-related configurations * Fix code quality per suggestion * Fix errors post-merge * Fix lint errors --------- Co-authored-by: Richard Dominick <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This pull request aims to allow users to configure on an assessment basis. This will allow finer control of each individual assessment's features, such as token counters and voting features.
Additionally, the admin panel will have general assessment configuration toggles for token counters and voting features. However, toggles such as Token Counter and Voting Features will only activate these features upon the creation of an assessment. Any further changes involving token counters and voting features must be done within ground control.
Type of change
Checklist