-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Quick edit modal for content title and description #4354
Quick edit modal for content title and description #4354
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.
This looks so great @AlexVelezLl! Nice work. Clean code, tests 🎉 , and manual testing looks good.
I have one piece of non-blocking feedback (although admittedly I am leaving this as a comment, rather than approving, just for the sake of having a discussion) which I would like the opinion of @bjester and @MisRob on, as this is a bit older in our "studio patterns".
We have some shared validators (I think Misha was someone who did some of the work on putting them into one place -- contentcuration/contentcuration/frontend/shared/utils/validation.js
) which we use in other places for form validation of various aspects of the content node data structure mostly.
@bjester and @MisRob - do you both think it would be valuable here to use those, for the sake of consistency? The title validation is functionally the same as what Alex has done...and perhaps he even referred to that to come up with the validation. Some of the validations are more complex, however, and in these cases it might be worth just importing and using the existing validators. And then, if we do that, for the sake of consistency, it might make sense to just use them throughout this project.
Alex, obviously none of this was specified in the issue, and we are just... figuring this out as we go. 😄 It has been a while since anyone has done any significant new front-end feature work in this part of Studio.
Anyone's thoughts welcome! (cc @akolson)
I think it'd be useful to have validators organized at one place (a file or a directory) since there are many of them across the Studio at various places. The current pattern was a reaction to having many similar/same validators all over the place. That said, I'm not sure if all existing validators will make sense for new work. For example here studio/contentcuration/contentcuration/frontend/shared/utils/validation.js Lines 170 to 176 in 525b7db
If existing validators can't be used, I still see value having them organized in one file even though they're a bit different, perhaps even more so, because we can at least make sure that validation rules are consistent, whether used for Vuetify or KDS components. |
Relatedly, we also have error messages in that file
EditTitleDescriptionModal.vue So that could be another reason to re-use the validation utils in one way or another.
|
v-if="showQuickEdit.titleDescription" | ||
:node="contentNode" | ||
@close="showQuickEdit.titleDescription = false" | ||
/> |
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.
Since this modal isn't specific to individual content nodes, or ContentNodeEditListItem
component itself, it seems slightly better to have it implemented outside of this component. As a modal, there should only ever be one of them open at a time. Having it implemented singularly outside of ContentNodeEditListItem
then better aligns its implementation with its behavior. You could have this component emit events upwards that can be handled where we render multiple of these as a group.
We have the same issue with ContentNodeContextMenu
but we've seen how that can increase memory usage and have come to recognize that its a pattern we should correct.
Hi @MisRob! I added a new method @bjester I pushed it up so that it is rendered two levels higher in the "CurrentTopicView" component, to have all the quick edit modals (also those for the bulk selection) in the same place. |
Oh that's so elegant and will be handy for other validators too! |
Yes this seems better, especially as you build more of these bulk editing modals! Thanks @AlexVelezLl |
27f437d
into
learningequality:studio-usability-enhancements
Summary
Description of the change(s) you made
Add a quick edit modal to edit content nodes titles and descriptions.
Screenshots
Compartir.pantalla.-.2023-12-08.17_14_21.mp4
Reviewer guidance
How can a reviewer test these changes?
References
Closes #3413
Comments
Contributor's Checklist
PR process:
CHANGELOG
label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocs
label has been added if this introduces a change that needs to be updated in the user docs?requirements.txt
files also included in this PRStudio-specifc:
notranslate
class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages
,components
, andlayouts
directories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarn
andpip
)