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

fix: fixed some read-only mode buttons bugs #832

Merged
merged 7 commits into from
Nov 29, 2021

Conversation

SystemChanger
Copy link
Contributor

@SystemChanger SystemChanger commented Jul 29, 2021

Fixes:

  1. "setToolbarButtons()" now correctly caches buttons to "resizingDisabledButtons" array.
    As it was: buttons were cached before being changed by this code:
let plugin, button, oldButton;
for (let key in pluginCallButtons) {
    if (!util.hasOwn(pluginCallButtons, key)) continue;
    plugin = plugins[key];
    button = pluginCallButtons[key];
    if (plugin.active && button) {
        oldButton = oldCallButtons[key];
        core.callPlugin(key, null, oldButton || button);
        if (oldButton) {
            button.parentElement.replaceChild(oldButton, button);
            pluginCallButtons[key] = oldButton;
        }
    }
}

As a result, they weren't disabled on the "readOnly()" invocations.

  1. "setToolbarButtons()" now don't enable buttons disabled by "readOnly()".
    As it was: On window resize (and other events with "setToolbarButtons()" invocation) buttons lost their "disabled" state.
    (check the second point in "Suggestions" below)

  2. Removed the "se-readonly-enabled" class from "Undo" and "Redo" buttons constructor.
    As it was: they had this class, but anyway were "returned" in their handlers cause of readOnly.
    Obviously, they don't need them.

  3. Now, read-only mode allows hiding more layer when the "more" button is pressed. Bonus: little code refactor.
    As it was: the "more" button didn't react on pressing and hiding the layer in read-only mode.

Suggestions: (#835 issue)

  1. Why "se-resizing-enabled" class and the corresponding array "resizingDisabledButtons" are called so?
    This is super strange, They should be called something like "se-readonly-enabled" and "readOnlyDisabledButtons".
    Version 3.0.0 ? :)

  2. I think that it is better to copy all properties of previous toolbar buttons to the new ones in "setToolbarButtons()" method, cause in the future there could be some other stuff besides read-only, fullscreen or codeView (or is already there).
    Check the issue ToolbarButtons code and functionality enhancements #835 for more details.

  3. In addition, it'll be good to have "onSetToolbarButtons(buttonList)" user-definable event function to declare the custom buttons css and properties (ofc we can override this method, but it'll be cleaner for users with the knowledge that they have to do this for custom buttons).
    UPD. Created a separate pull request for "onSetToolbarButtons()" event:
    enhancement: added "onSetToolbarButtons" event #834

@SystemChanger SystemChanger changed the title fix: fixed some read-only mode button display bugs fix: fixed some read-only mode buttons display bugs Jul 29, 2021
@SystemChanger
Copy link
Contributor Author

SystemChanger commented Jul 29, 2021

Fixes:

  1. Now closes active menus, which buttons were disabled by setting the read-only.
  2. As it was: menus buttons could be pressed even if their parents (Dropdown buttons) were disabled.
  3. Now, custom buttons designed for read-only (with "se-resizing-enabled" class) can invoke handlers in read-only mode. As it was: custom buttons (plugins) weren't able to invoke their callBackFunctions regardless of the presence of the "se-resizing-enabled" class.

We are getting close to the reason I started this all "read-only" adjustments ;)

@SystemChanger SystemChanger changed the title fix: fixed some read-only mode buttons display bugs fix: fixed some read-only mode buttons bugs Jul 30, 2021
if (core.hasFocus) event._applyTagEffects();

if (core.isReadOnly) util.setDisabledButtons(true, core.resizingDisabledButtons);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need in this line if you will apply the changes from #839 pull-request

@JiHong88
Copy link
Owner

@SystemChanger Thank you for your contribution.!
"se-resizing-enabled" is a variable created before "readonly" mode development.
These are buttons that are not deactivated when selecting images, etc.
It is also used in "readonly" mode.

@JiHong88 JiHong88 merged commit deebd72 into JiHong88:master Nov 29, 2021
@JiHong88
Copy link
Owner

@SystemChanger The 2.42.0 version has been updated.
If this issue has not been resolved, please reopen this issue.
Thank you.

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.

2 participants