feat: move activity types new and edit to mui#977
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
@priscila-moneo looks good ! just one thing
| linkToPresentationType, | ||
| unlinkFromPresentationType | ||
| }) => { | ||
| const [isSaving, setIsSaving] = useState(false); |
There was a problem hiding this comment.
please use baseState loading prop . it is set at the beginning of each action call: example
| linkToPresentationType, | ||
| unlinkFromPresentationType | ||
| }) => { | ||
| const isSaving = Boolean(loading); |
There was a problem hiding this comment.
isSaving is derived from the global baseState.loading counter instead of local state. This means any concurrent request (autocomplete, list reload, etc.) incorrectly disables the form while the user is filling it in.
The canonical pattern for popups in this project (see summit-admin-popup-dialog-pattern.md and the media-file-type-dialog.js reference) requires:
const [isSaving, setIsSaving] = useState(false)in the popupsetIsSaving(true)before calling save.finally(() => setIsSaving(false))to reset
Related: saveEventType is connected directly via Redux-connect in this dialog (mapDispatchToProps line 132), which is also an anti-pattern per the project rules — the save action belongs in the parent's handleSave, passed down as a single onSave prop.
see https://app.clickup.com/t/9014802374/86bag2zk7
| dispatch( | ||
| showSuccessMessage(T.translate("edit_event_type.event_type_saved")) | ||
| ); | ||
| return dispatch(getEventTypes()); |
There was a problem hiding this comment.
The list reload (dispatch(getEventTypes())) belongs in the parent's handleSave, not inside the action. As-is, saveEventType is coupled to the list page: it can't be reused in any other context without triggering a full list refresh.
The canonical shape (from the media-file-type-list-page.js reference and ticket 86bag2zk7) is:
// parent
const handleSave = (entity) =>
saveEventType(entity)
.then(() => getEventTypes(term, DEFAULT_CURRENT_PAGE, perPage, order, orderDir));Same issue applies to the POST path on line 155.
priscila-moneo
force-pushed
the
feature/move-activity-types-grid-mui
branch
from
77cea30 to
6872754
Compare
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/move-edit-new-activity-types-mui
branch
from
82c99d3 to
bb7892e
Compare
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
3 participants
ref: https://app.clickup.com/t/9014802374/86bacr6x8