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

Set this.changingState back to false in ExtensionPage if an error occurs #2558

Merged

Conversation

dsevillamartin
Copy link
Member

@dsevillamartin dsevillamartin commented Jan 20, 2021

Fixes #2536

Changes proposed in this pull request:

  • Set this.changingState = false in onerror handler.
  • Only use this.changingState for Switch status & its loading attribute
    • do not take it into account in whether to show the ext settings as that makes it look bad (see reviewer focus below)
Reviewers should focus on:

I feel like we should instead get rid of changingState completely. It causes the UI to glitch out and there's no reason for the text & info shown to update while the "Please wait" modal is active and then be wrong.

hfdRh7iPce.mp4

Video:

UmCttTKrm5.mp4

Confirmed

  • Frontend changes: tested on a local Flarum installation.

@askvortsov1
Copy link
Member

askvortsov1 commented Jan 20, 2021

I feel like we should instead get rid of changingState completely. It causes the UI to glitch out and there's no reason for the text & info shown to update while the "Please wait" modal is active and then be wrong.

I think I would prefer this too: we have a continuous stream of animation via the loading modal, so it's not like users wouldn't get visual confirmation that they're trying to activate/deactivate an extension.

That being said, it would be nice to keep the header switch itself enabling; perhaps changingState could be moved out of isEnabled, and directly into the "checked" criteria for the switch?

@dsevillamartin
Copy link
Member Author

dsevillamartin commented Jan 23, 2021

That being said, it would be nice to keep the header switch itself enabling; perhaps changingState could be moved out of isEnabled, and directly into the "checked" criteria for the switch?

I like this. I also set it to the loading attr because as we don't want the Enabled/Disabled text changing, and in case there's an issue with the modal not appearing or something, the user knows that it's possibly not fully enabled/disabled and it was in progress.

@dsevillamartin
Copy link
Member Author

dsevillamartin commented Jan 23, 2021

This currently has an issue - the switch does not go back to the original state after the error.

Nevermind, there's no issue. I removed the local dist changes and the original issue popped up 😂

@askvortsov1 askvortsov1 merged commit a488045 into master Jan 30, 2021
@askvortsov1 askvortsov1 deleted the ds/2536-extension-page-reset-changing-state-on-error branch January 30, 2021 22:43
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.

When an extension dependency is not met, the extension page still switches to enabled
3 participants