New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#6800 Page Editor Walkthrough Modal #6894
Conversation
The modal approach is looking good.
Is there a reason we're using images for the shortcuts keys vs. using our Shortcut Keys component?:
@BrandonPxBx a simpler shortcut is F12. Did we want to include that shortcut as well? |
QQ: Did you run the svgs through an optimizer? |
@grahamlangford was having trouble getting typescript to recognize the webp format, so due to time constraints I'm going to use png's in place of the hefty svgs, see https://github.com/pixiebrix/pixiebrix-extension/actions/runs/6897518554/job/18765844476 Happy to discuss things I tried later if you'd like to dig in; I checked the tsconfig and experimented with webpage config as well |
…github.com/pixiebrix/pixiebrix-extension into feature/6800_page_editor_walkthrough_modal merge main
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.
Conditionally approved: see comment on splitting out the context script vs. walkthrough code: https://github.com/pixiebrix/pixiebrix-extension/pull/6894/files#r1396636873
When the PR is merged, the first loom link found on this PR will be posted to |
} finally { | ||
reportEvent(Events.PAGE_EDITOR_WALKTHROUGH_MODAL_CLOSE); | ||
controller.abort(); | ||
} |
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 promise/controller setup should be reviewed, I see several issues with it:
modal
can't throw because we control it, and it only calls.resolve
, so no need for try/catch- The controller and the promise are doing the same thing (closing the modal), so they should be merged (e.g.
registerWalkthroughModal
should return thesignal
) - If
showModal
is managing the modal, why are its registration and abort not also handled by it? - AbortControllers aren't meant to be passed around, there should be one controller that allows for listening via the
signal
I think this should look like:
- const controller = new AbortController();
- const modal = registerWalkthroughModal();
- showModal({ url: frameSource, controller });
- try {
- await modal;
- } finally {
- reportEvent(Events.PAGE_EDITOR_WALKTHROUGH_MODAL_CLOSE);
- controller.abort();
- }
+ await showModal({ url: frameSource });
+ reportEvent(Events.PAGE_EDITOR_WALKTHROUGH_MODAL_CLOSE);
What does this PR do?
PageEditorWalkthroughModalView
andPageEditorWalkthroughModalClose
eventsDemo
Demo of the feature in draft is here: https://www.loom.com/share/ed5a86a64c5848cdb4431082c4c06eb3
Final demo: https://www.loom.com/share/0460d74a100346faac50a43e6ac21803
Checklist