-
Notifications
You must be signed in to change notification settings - Fork 113
refactor(core): modernize toggleFullScreen and synchronize state updates
#244
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
refactor(core): modernize toggleFullScreen and synchronize state updates
#244
Conversation
brunob
left a comment
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.
As usual, thx for the detailed PR :)
|
I hope it's not annoying that I'm creating so many PRs! But I think it's worth it. |
|
I certainly won't blame people that give of their free time and provide help and such relevant contributions ! You can't imagine how i'm happy to see collaboration emerge on this project :) |
|
Any thought @BePo65 ? |
355a470 to
1f6dce3
Compare
- Convert toggleFullScreen to async/await for better readability - Wait for fullscreen transition before updating state and firing events - Simplify logic by centralizing element and mode detection - Simplify _handleFullscreenChange detection for external exits (ESC key) - Update state before firing events for correct event handler behavior
1f6dce3 to
05d25dc
Compare
|
Thx again, if it's ok for you let's wait January to push a new release (?) |
|
If it's okay, a last release for this year would be nice, as the type definitions would then be included 🙂 |
|
Even for me all these changes look very good. |
I will be unavailable next two weeks, so that's why i prefer to release after this break in way to be present if anything is broken with latest changes (but i'm sure it will be ok :)) |
|
Makes sense, no problem. Have a nice time! 🙂 |
|
Hello @KristjanESPERANTO, |
|
@mickaeltr Thanks for the hint. I will have a look in the next days 🙂 |
|
@KristjanESPERANTO should we release a new version now, or wait a bit until you have time to take care of @mickaeltr hints ? (no hurry) |
|
Et voilà, version 5.2.0 is published :) |
|
Nice! And I've created a PR to remove the types for leaflet.fullscreen from DefinitelyTyped: DefinitelyTyped/DefinitelyTyped#74316 🙂 Since the types are now here in the repo, they are superfluous there. |
The method now uses
async/awaitinstead of Promise chains (.then()), making the control flow more linear and easier to follow.Additionally, the execution order was improved: the fullscreen transition now completes before updating
map._isFullscreenand firing events. This ensures that event listeners always receive the correct, up-to-date state.A nice side effect of this change is that the code in
_toggleStatenow reads more intuitively:Previously, the logic was inverted because the state hadn't been updated yet when the event fired.
Summary of changes:
toggleFullScreentoasync/awaitfor better readability._handleFullscreenChangedetection for external exits (ESC key)._toggleStatelogic to align with the new execution order.Note on
_handleFullscreenChangeThe new execution order required adjusting this method: since
map._exitFiredis now set beforeexit()(not after), the condition needed to also checkmap._isFullscreento correctly detect external exits (ESC key).This is a polish PR - there are no functional changes, but the code should now be easier to understand and maintain.