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
Assignment Updates: assign button assigns a course #31385
Conversation
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.
LGTM Erin. One question: Why did you switch away from the AssignedButton?
Good question. I'm getting nervous about the on-hover unassign UI since it's inconsistent with other UI across the site so I swapped it to show Mark and Amanda. We might to keep it like this and add a separate in-line unassign button or switch back to the hover way it was. |
apps/test/unit/code-studio/components/libraries/LibraryCreationDialogTest.js
Outdated
Show resolved
Hide resolved
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.
Great work Erin!
{ | ||
assignCourseToSection | ||
} |
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.
cool! I did not know about this syntax: https://react-redux.js.org/using-react-redux/connect-mapdispatch#defining-mapdispatchtoprops-as-an-object
{selectedSection.isAssigned && ( | ||
<span style={styles.assigned}> | ||
<FontAwesome icon="check" /> | ||
{i18n.assigned()} | ||
</span> | ||
)} |
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.
The new UX looks good to me!
@@ -140,6 +140,20 @@ export const toggleSectionHidden = sectionId => (dispatch, getState) => { | |||
return dispatch(finishEditingSection()); | |||
}; | |||
|
|||
/** | |||
* Assigns a course to a given section, persisting these changes * to the server |
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.
nit: stray *
near end of line
dispatch(beginEditingSection(sectionId, true)); | ||
dispatch(editSectionProperties({courseId: courseId})); | ||
return dispatch(finishEditingSection()); |
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.
very creative! this is a bit roundabout, since it is piggybacking off of actions that are designed for the edit section form. but as long as we are only doing actions that are a subset of what we can do in that form, then it seems like this should work fine (and also later for assigning scripts).
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.
Looks good!
Description
This PR leverages existing edit section details functions from teacherSectionsRedux so that the
AssignButton
actually assigns a course in the context of the course overview page to the section selected from the dropdown.Links
Testing story
Reviewer Checklist: