-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Migrate preview-panel.js
to Stimulus PreviewController
#10356
base: main
Are you sure you want to change the base?
Conversation
Manage this branch in SquashTest this branch here: https://laymonagepreview-panel-control-s7sqa.squash.io |
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.
One comment, happy to help will a full review later if that helps.
Super great to see another bit of Stimulus work, hopefully the code feels easier to manage this way.
2088cf6
to
550f49b
Compare
preview-panel.js
to Stimulus PreviewPanelController
preview-panel.js
to Stimulus PreviewController
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.
A few more drive-by comments.
550f49b
to
118748e
Compare
118748e
to
c7ace09
Compare
c7ace09
to
5403077
Compare
f44d4cd
to
c432818
Compare
Thanks for the comments @lb-, I believe I've addressed those. I think this is only missing unit tests before it's ready for review, will try to write them over the week. |
Thanks @laymonage - happy to do a full review when ready. |
this.handlePreview(); | ||
} | ||
|
||
connect() { |
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.
Not strict but it's nice to have the connect method at the top (after declaring properties), as it's kind of the entry point to the controller.
'newTabButton', | ||
'spinner', | ||
'modeSelect', | ||
'iframe', |
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.
Just thinking it may be good to streamline these target names, removing the element type from the reference name + keeps it shorter.
The fact that it's an input/button is probably a duplicate as the html elements that are the target will be these types.
static targets = [
'size',
'newTab',
'spinner',
'mode',
'iframe',
Also, not strict, but we have been sorting these alphabetically in other controllers.
@laymonage I needed the Promise handling of the I have adjusted the behaviour for the |
No worries @lb-, thanks for making the |
Without typing Promise and Promise.resolve with R, it defaults to unknown, so the R generic type can't be used. Also, the debounced function's args should take the generic A parameter instead of redefining it as any[].
This allows the use of the button-longrunning handling for the loading state. Also, turn the button into an icon-only button as there might not be enough space when the panel is resized to the smallest size
If the event is dispatched in the loop, then the show event may be dispatched before the hide event is dispatched. For example, if you're switching from the checks panel to the preview panel, as the preview panel is ordered before the checks panel in the DOM. This could cause an issue with the preview panel code, where we listen for the show event to activate the auto update, and listen to the hide event to deactivate it. We're listening to both the preview and checks side panels. Without this fix, the preview auto-update will get deactivated upon switching from the checks panel to the preview panel, as the hide event is dispatched after the show event.
Test loading the last device size from localStorage
This allows developers to more easily customise the panel, e.g. customising the device sizes
3fb51f8
to
707b45d
Compare
I've been playing around with Stimulus by refactoring the preview panel and I must say I quite enjoyed it! I think Stimulus fits nicely with our JavaScript use case in the front-end.
Fixes #9360 and adds some more optimisation by debouncing the update. It's still in draft as I have yet to write tests and do more testing.
Fixes #11677.
Please check the following:
make lint
from the Wagtail root.Please describe additional details for testing this change.
Footnotes
Development Testing ↩
Browser and device support ↩
Accessibility Target ↩