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

[Modal] "show" does not recenter modal #2920

Open
mvorisek opened this issue Oct 8, 2023 · 3 comments
Open

[Modal] "show" does not recenter modal #2920

mvorisek opened this issue Oct 8, 2023 · 3 comments
Assignees
Labels
state/has-pr An issue which has a related PR open type/bug Any issue which is a bug or PR which fixes a bug
Milestone

Comments

@mvorisek
Copy link
Contributor

mvorisek commented Oct 8, 2023

Bug Report

Steps to reproduce

  1. open https://dev.atk4.org/demos/_unit-test/late-output-error.php
  2. click first button (Test LateOutputError I: Headers already sent)
  3. notice the modal is NOT vertically centered

The demo closes another modal right before the final modal is shown, but when I put $(this).modal('refresh'); at https://github.com/atk4/ui/blob/5.0.0/js/src/services/modal.service.js#L44 line the problem is gone, so it seems the "show" does not explicitly refresh the modal at all.

Expected result

.modal('show') must always refresh/recenter the modal

Screenshot (if possible)

image

Version

2.9.3

@mvorisek mvorisek added state/awaiting-investigation Anything which needs more investigation state/awaiting-triage Any issues or pull requests which haven't yet been triaged type/bug Any issue which is a bug or PR which fixes a bug labels Oct 8, 2023
@ko2in ko2in removed the state/awaiting-triage Any issues or pull requests which haven't yet been triaged label Oct 10, 2023
@mvorisek
Copy link
Contributor Author

mvorisek commented Oct 15, 2023

The repro demo does no longer reproduce the issue, as we fixed it on our side temporary: https://github.com/atk4/ui/blob/7cc2e65c144480962433dd4aef5ef351380dace2/js/src/services/modal.service.js#L48

To reproduce it, download the demo (html, js, + the loaded page using AJAX) locally and remove the fix.

It seems the "refresh on show" was removed in https://github.com/fomantic/Fomantic-UI/pull/2477/commits (in last commit) - what about reverting it (reintroducing forced refresh on show) until #2476 is fixed?

UPDATE: I looked into the latest JS code and please have a look at #2476 (comment) repro. The repro is changing classes not only style, and such changes should be observeable and causing always a refresh when observeChanges: true.

@lubber-de
Copy link
Member

UPDATE: I looked into the latest JS code and please have a look at #2476 (comment) repro. The repro is changing classes not only style, and such changes should be observeable and causing always a refresh when observeChanges: true.

Always triggering a refresh on any classname change within the whole modal will almost always trigger a refresh especially for modules like dropdown or other elements which make use of the transition module / classes. I fear a big performance lag here then. Especially as the "refresh()" method itself also changes classnames inside the modal (e.g. the "scrolling" or "loading" class)

@lubber-de lubber-de self-assigned this Oct 17, 2023
@lubber-de lubber-de added this to the 2.9.4 milestone Nov 17, 2023
@lubber-de lubber-de removed the state/awaiting-investigation Anything which needs more investigation label Nov 17, 2023
@lubber-de
Copy link
Member

Should be fixed by #2969. The new Resizeobserver automatically triggers a refresh when a modal gets shown so a separate refresh on show isnt needed

@lubber-de lubber-de added the state/has-pr An issue which has a related PR open label Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state/has-pr An issue which has a related PR open type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

No branches or pull requests

3 participants