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

hasan-ozbey
Copy link
Contributor

@hasan-ozbey hasan-ozbey 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).

Copy link
Member

@dsevillamartin dsevillamartin left a comment

Choose a reason for hiding this comment

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

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!

@hasan-ozbey
Copy link
Contributor Author

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
Contributor

@tankerkiller125 tankerkiller125 left a comment

Choose a reason for hiding this comment

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

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

@hasan-ozbey
Copy link
Contributor Author

hasan-ozbey 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
@luceos
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 😞

@hasan-ozbey
Copy link
Contributor Author

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
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!

@hasan-ozbey hasan-ozbey deleted the the-turk-patch-1 branch March 31, 2020 15:30
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.

Modal is not showing Start a Discussion Tag Popup closes in IE 11
4 participants