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 Buggy Modals #2080

Merged
merged 3 commits into from Mar 30, 2020
Merged

Fix Buggy Modals #2080

merged 3 commits into from Mar 30, 2020

Conversation

@the-turk
Copy link
Contributor

the-turk commented Mar 21, 2020

Fixes #1504
Fixes #1813

Changes proposed in this pull request:

First commit was proposing to change margin values for @tablet-up breakpoint as seen on Bootstrap stylesheets. Evethough it fixes the issue, i wasn't happy with it and did some further research. Eventually, i noticed that we're calling onready() method of ModalManager component twice [Line 33 & 57]. First call waits for transitions to be complete and the second one seems useless. Made some conversation with @datitisev on Discord and now we're here.

Reviewers should focus on:

  • Did i miss something?

I know this one is hard to catch since it seems sporadic but try to test it with developer tools docked to bottom of the browser (make it bigger until you succeed).

Note that zooming out from the page before clicking on the "Choose Tags" button also fixes the issue (as stated by @sometao) so it should be something related with viewport heights.

I look forward to your reviews and I need to fix this since it effects some of my modal related extensions.

Screenshot

https://vimeo.com/399523797

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
the-turk added 3 commits Mar 21, 2020
Copy link
Member

datitisev left a comment

I tested this change locally, and it worked perfectly.

To reproduce, I had to open the developer tools and make them extremely tall. Not sure why the size of the available space causes issues with Bootstrap 3's modal plugin, but onready shouldn't be called twice anyway (see line 33).

Thanks for your effort!

@the-turk

This comment has been minimized.

Copy link
Contributor Author

the-turk commented Mar 22, 2020

This doesn't fix extension settings modal's issue.

People who encounters with this issue may see that the same thing happens if they enable an extension which has settings modal (it'll try to show the extension's settings modal after enabling it but it'll fail eventually).

Here are some quotes from @datitisev:

both timeout of setTimeout(() => this.extensionSettings[enabled](), 0); or requestAnimationFrame(() => this.extensionSettings[enabled]()) work

only thing is that it blinks with full "brightness" before opening the modal, but not much we can do about that

Copy link
Member

tankerkiller125 left a comment

I also just tested locally using the instructions provided by @datitisev to replicate the issue and this PR fixes the issue.

@the-turk

This comment has been minimized.

Copy link
Contributor Author

the-turk commented Mar 27, 2020

Some modal issues are still a mystery, i had the same missing class problem while developing the Diff extension and i end up adding it manually.

Maybe we should accept this PR as a solution for the tag selection modal issue (or we can just call it "a fix for double onready() call") and check other modal issues (like extension settings) after the frontend rewrite.

@luceos luceos merged commit 6bbd603 into flarum:master Mar 30, 2020
10 checks passed
10 checks passed
PHP 7.2 / MySQL
Details
PHP 7.2 / MariaDB
Details
PHP 7.3 / MySQL
Details
PHP 7.3 / MariaDB
Details
PHP 7.4 / MySQL
Details
PHP 7.4 / MySQL (prefix)
Details
PHP 7.4 / MariaDB
Details
PHP 7.4 / MariaDB (prefix)
Details
WIP Ready for review
Details
continuous-integration/styleci/pr The analysis has passed
Details
@luceos

This comment has been minimized.

Copy link
Member

luceos commented Mar 30, 2020

Thanks for this fix. I just merged and deployed it, but seemingly it doesn't fix the issue with the edit tags modal 😞

@the-turk

This comment has been minimized.

Copy link
Contributor Author

the-turk commented Mar 30, 2020

Thanks for this fix. I just merged and deployed it, but seemingly it doesn't fix the issue with the edit tags modal 😞

@luceos I can confirm that this PR fixed both modals

@luceos

This comment has been minimized.

Copy link
Member

luceos commented Mar 30, 2020

@the-turk my apologies, it seems I had been too fast with updating discuss. Compilation of the dist files hadn't been completed yet, I can confirm the PR works, thank you!

@the-turk the-turk deleted the the-turk:the-turk-patch-1 branch Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.