refactor(core): modernize toggleFullScreen and synchronize state updates#244
Conversation
brunob
left a comment
There was a problem hiding this comment.
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.