#8607 More replacing element -> modComponentFormState #8706
Conversation
…mponentTraceForBrick
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8706 +/- ##
==========================================
- Coverage 73.69% 73.64% -0.06%
==========================================
Files 1344 1346 +2
Lines 41566 41604 +38
Branches 7775 7797 +22
==========================================
+ Hits 30633 30638 +5
- Misses 10933 10966 +33 ☔ View full report in Codecov by Sentry. |
| @@ -23,14 +23,14 @@ import { useDispatch, useSelector } from "react-redux"; | |||
| import useResetExtension from "@/pageEditor/hooks/useResetExtension"; | |||
| import { selectModComponentFormStates } from "@/pageEditor/slices/editorSelectors"; | |||
|
|
|||
| function useResetRecipe(): (recipeId: RegistryId) => Promise<void> { | |||
| function useResetRecipe(): (modId: RegistryId) => Promise<void> { | |||
There was a problem hiding this comment.
Should we go ahead and rename this hook as well? I don't think it's used in very many places.
There was a problem hiding this comment.
Changing recipe -> mod is sort of out of scope. I stopped myself here because I realized that the renaming could easily spiral if I kept following the thread of getting all "recipe" occurrences. We have a ticket for getting rid of recipe in the Page Editor
.... or at least I thought we did? I might have to open an additional ticket to #8579
There was a problem hiding this comment.
Made a ticket here: #8710
Playwright test results
Details
Skipped testschrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options |
| */ | ||
| export function selectNodePreviewActiveElement( | ||
| export function selectActiveDocumentOrFormPreviewElement( |
There was a problem hiding this comment.
I don't love the "DocumentOrFormPreviewElement" term and wonder if we can figure out something simpler, like "DOMElement" or something, but probably not worth considering in this PR 🤔
There was a problem hiding this comment.
I don't either, but unless we combine the concepts of "DocumentBuilder" and "FormBuilder" I can't think of an alternative that communicates the same thing to this effect.
There was a problem hiding this comment.
I don't either, but unless we combine the concepts of "DocumentBuilder" and "FormBuilder" I can't think of an alternative that communicates the same thing to this effect.
Would BuilderElement work? That would capture both
We don't have any near-term plans to combine the document builder and form builder
There was a problem hiding this comment.
Would BuilderElement work?
Maybe just PreviewElement in that case? e.g. just selectActivePreviewElement This particular piece of data is in reference to the preview specifically.
There was a problem hiding this comment.
Maybe just PreviewElement in that case? e.g. just selectActivePreviewElement This particular piece of data is in reference to the preview specifically.
Yeah that works for me if you're confident it's also an appropriate name here, I would say just go with your best judgement 🤷♂️
There was a problem hiding this comment.
Would you mind if I opened up a separate PR to revert this? Past PRs introduced this naming pattern and theres a lot of places I use DocumentOrFormPreviewElement
There was a problem hiding this comment.
Added a comment to this ticket #8609 to address this issue
There was a problem hiding this comment.
Yeah totally cool to be a separate PR 👍
| isAutomaticTrigger(modComponentFormState) || // By default, don't automatically trigger (because it might be doing expensive operations such as hitting an API) | ||
| isPanel(modComponentFormState) || |
There was a problem hiding this comment.
| isAutomaticTrigger(modComponentFormState) || // By default, don't automatically trigger (because it might be doing expensive operations such as hitting an API) | |
| isPanel(modComponentFormState) || | |
| isAutomaticTrigger(modComponentFormState) || | |
| // By default, don't automatically trigger (because it might be doing expensive operations such as hitting an API) | |
| isPanel(modComponentFormState) || |
There was a problem hiding this comment.
Why put the comment underneath isAutomaticTrigger?
There was a problem hiding this comment.
I put it inline because prettier adds gross extra parenthesis when I put it on the line above isAutomaticTrigger
There was a problem hiding this comment.
It looks to me like in the previous code before your PR, this comment was meant for the isPanel line and not the isAutomaticTrigger line?
https://github.com/pixiebrix/pixiebrix-extension/blob/main/src/pageEditor/toolbar/ReloadToolbar.tsx#L72-L75
There was a problem hiding this comment.
Technically, I think the comment is referring to how we check for !isPanel() when deciding whether to "auto update".
There was a problem hiding this comment.
It looks to me like in the previous code before your PR, this comment was meant for the isPanel line and not the isAutomaticTrigger line?
Oh, it looks like I got confused by the "automatic trigger" terms in the comment
# Conflicts: # src/pageEditor/hooks/useSaveMod.ts
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
What does this PR do?
useSavingWizard(🎉) which I noticed is unused save for a utility func I moved