Skip to content
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

Bugfix: Correctly keep preview active during fullscreen-toggle #316

Merged
merged 16 commits into from Mar 27, 2021

Conversation

smundro
Copy link

@smundro smundro commented Mar 16, 2021

This is a fix for issues created by #286, as noted in #306.

The initial attempt to keep preview active when toggling full-screen, was to simply not call toggleSideBySide(), which unfortunately didn't account for "no-fullscreen" classes that are applied in the non-fullscreen side-by-side preview mode.

In this PR:

  • move to applying.sided--no-fullscreen only to the parent element and using CSS inheritance
  • on full-screen toggle, add/remove .sided--no-fullscreen as needed

For future refactoring, it would be cleaner to apply general status classes to the .EasyMdeContainer, such as fullscreen sided and have the sub-elements key off the parent class to avoid having to set classes on multiple elements. This would allow all the .EasyMdeContainer.sided--no-fullscreen ... classes be replaced with .EasyMdeContainer.sided:not(.fullscreen), with similar changes throughout the rest of the code (.editor-toolbar.fullscreen to .EasyMdeContainer.fullscreen .editor-toolbar, etc.).

@smundro smundro changed the title Bugfix: Correctly handle keeping preview active during fullscreen-toggle Bugfix: Correctly keep preview active during fullscreen-toggle Mar 16, 2021
@smundro
Copy link
Author

smundro commented Mar 17, 2021

@Ionaru Anyone using sideBySideFullscreen:false and allowing full-screen toggling will be seeing issues due to my previous change, this fix should be deployed ASAP.

@Ionaru
Copy link
Owner

Ionaru commented Mar 17, 2021

It's a big change with lots of code changes, it's going to take a little while to make sure everything still works as it's supposed to.

Note that this isn't necessary and can be rolled back, but could be used throughout the code to make class handling more clear and consistent.
@smundro
Copy link
Author

smundro commented Mar 17, 2021

Understood. Fundamentally, there's only two changes:

  1. remove the '.sided--no-fullscreen' class application to child elements : I verified that this doesn't have any side-effects in the code base, the only concern is if anyone using the package is applying their own styles based on the child element classes.
  2. applying the '.sided--no-fullscreen' class when toggling full-screen: This was the piece I missed in my original change, when toggling full-screen the classes need to be changed when the side-by-side is open.

I did also just add some add/removeClass convenience functions, because the string manipulation of classNames is a common pattern, but that can be reversed.

@Ionaru Ionaru merged commit 113c5b4 into Ionaru:master Mar 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants