Split out editing venues from editing schedule #10028
Conversation
both point to the same react component
not actually doing anything, as far as I can tell
since it doesn't exist anymore
app/webpacker/lib/utils/wcif.js
Outdated
|
|
||
| save(url, wcifData, onSuccess, options, onError); | ||
| }, | ||
| [save], | ||
| [save, patchOpts], |
There was a problem hiding this comment.
You're passing patchOpts as an object (useSaveWcifAction({ skipSchedule: true })) which means this will be a fresh object (reference) each render, so this saveWcif will re-compute every time, negating the use of useCallback. If any code is relying on saveWcif never re-computing (i.e. using saveWcif in a dependency array somewhere), then this will break that code.
This can be avoided by defining const wcifSaveOptions = { skipSchedule: true } outside the component. But it doesn't prevent anyone in the future from passing in an object 'incorrectly.'
(I'm not sure if the default value {} is a fresh object each time?)
Making the options a parameter to save, instead of the hook itself, might work.
There was a problem hiding this comment.
Fixed! Should now be less recompute-heavy. And this incidentally also fixes the error with the backend not saving, because the PATCH request was accidentally passing undefined in the URL under certain circumstances.
There was a problem hiding this comment.
Everything looks good to me, and works well as far as I can tell.
I wonder if specifying what should be saved, rather than what should not be saved, might be better though? In the past I have been editing groups in delegate dashboard, and also editing the schedule on the site, and overwritten myself, for example.
There was a problem hiding this comment.
No wait, adding rooms might still cause conflicts when editing in different browser tabs. I will investigate more tomorrow. But your changes certainly haven't made anything worse.
There was a problem hiding this comment.
Things that can go wrong with 2 tabs open for venues/schedule respectively:
- Delete a venue/room & save, then edit the schedule in that room & save -> 'invalid json' error
- Copy an existing room (in venue editor) & save, then edit anything in the schedule & save -> new copied room is gone
- Maybe other scenarios
Simple things like editing the fields of venues/rooms while editing the schedule seem to work fine, as far as I can tell.
The styling is a bit off with the "Venues + Add a venue" and the text at the top of each tab being a bit close to the content below (formerly there was an accordion).
Now each folder has an
indexand a (single) component subfolder with anotherindex, so it might be worth bringing those subfolder contents up and having a singleindex(maybe as a follow-up PR?).There's some minor duplication now, since everything was duplicated then trimmed away, mostly in the 2 top-level
indexfiles.I removed a scss file because it didn't appear to be doing anything, but I'm not 100% confident in this.