Can copy (or rename or delete) on ClassEditPage as well #13771
Conversation
github-actions
bot
added
APP: Facility
Build Artifacts
|
nucleogenesis
added
the
TODO: needs decisions
There was a problem hiding this comment.
Thanks @nucleogenesis! Just a couple of nitpick comments :)
| return [ | ||
| { | ||
| label: this.copyClass$(), | ||
| value: 'COPY_CLASS', | ||
| id: 'copy', | ||
| }, | ||
| { | ||
| label: this.renameClassLabel$(), | ||
| value: 'EDIT_CLASS_NAME', | ||
| id: 'rename', | ||
| }, | ||
| { | ||
| label: this.deleteClass$(), | ||
| value: 'DELETE_CLASS', | ||
| id: 'delete', | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
I don't think we need the id property for KDropdownMenu options, and we would probably want to use the Modals.DELETE_CLASS constants instead of the magic strings.
There was a problem hiding this comment.
Admittedly I just copied this from the ManageClassPage component. I was fully under the impression that the ID was needed but then I look at KDS and there's no use of id in UiPopover or KDropdownMenu. Makes me think I'm mixing a couple things up.
| .title-and-options { | ||
| display: flex; | ||
| flex-direction: row; |
There was a problem hiding this comment.
Just noting that row is the deafult value for flex-direction, so we usually don't need to set it explicitly 😅.
There was a problem hiding this comment.
I used the Chrome devtools little flexbox helper thing to apply the styles til it looked right then copy-pasted the rules - was a little aggressive setting each option lol
| :callback="goToClassesPage" | ||
| @cancel="clearClassToDelete" | ||
| @success="clearClassToDelete" |
There was a problem hiding this comment.
Are there benefits of passing the goToClassPage callback as a prop, instead of using it in the @success event? I see that this callcback is always being called in the only place where the ClassDeleteModal component emits the @success event.
There was a problem hiding this comment.
Great catch - I did the callback first in the copy modal which didn't emit success then just quickly applied the same to the other modal where I needed it.
Just pushed a commit reverting the callback props and handling the "go back" stuff in the success event handler.
- Move ClassRenameModal, ClassCopyModal, and ClassDeleteModal to common folder - Move useDeleteClass composable to composables directory - Update all import references to point to new locations - Add deleteClass string to bulkUserManagementStrings and update imports - Fix copy class modal functionality on ClassEditPage - Add dropdown menu with copy, rename, and delete options to class edit page - Add callback support for navigation after modal operations where needed 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
nucleogenesis
force-pushed
the
fix--copy-class-on-edit-page
branch
from
b5500ed to
4c3feea
Compare
marcellamaki
left a comment
marcellamaki
left a comment
There was a problem hiding this comment.
Did a quick code re-review and some extra manual QA -- this is good to go, and we'll have the QA team take a closer look when testing beta0
3 participants
Summary
Done w/ help of Claude Code, primarily for the tedious file moving & import renaming.
Addressing #13770 - this takes a very opinionated approach to the UX that will allow the user to copy. Basically, I went to figure out where I'd put a "Copy this class" button to trigger the modal and thought that if I'm putting a button somewhere I might as well make the UX consistent w/ the context dropdown on the classes table.
The thing I think might be a bit contentious is that I made both the "Assign Coaches" and the "Enroll Learners" buttons into Primary buttons to visually distinguish them from the secondary-style Options dropdown button.
References
Fixes #13770
Reviewer guidance
DESIGN & UX
CODE
QA