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 modal regression #2468

Merged
merged 2 commits into from
Jul 23, 2024
Merged

Fix modal regression #2468

merged 2 commits into from
Jul 23, 2024

Conversation

samuelhwilliams
Copy link
Contributor

@samuelhwilliams samuelhwilliams commented Jul 21, 2024

In #2407 we removed some seemingly-duplicate network requests for
bs4_modal.js every time a modal window is loaded.

This would have been fine if that file did not include any
side-effects, but it did: it re-applied 'global styling' every time the
JS file is loaded.

The goal here was to process 'styling' for any dynamic/interactive
elements, such as map widgets, and fields that use 'select2' or
'x-editable'.

When we removed these 'extra' JS calls, the modal windows stopped
getting these interactive elements enabled/setup.

This patch restores that functionality, while still retaining the goal
of PR #2407: to prevent duplicate network requests for a file.

We load bs<x>_modal.js on all pages that can display modals; this file
now only sets up the event listener that is responsible for making sure
a modal is cleared out/refreshed between multiple modal displays.

The logic for applying styling to the window is now added inline to all
of the relevant modal components.

A further improvement on what we had before is that now, the call to add
styling is scoped specifically to the modal. This prevents the issue
described in #2351, where map elements may show up multiple times on
the page because applyGlobalStyles is called multiple times.

fixes: #2351


Current master branch - modals are broken

2024-07-21 20 44 40

Current master branch with #2407 reverted - multiple elements; gets worse over time

2024-07-21 20 45 56

After this PR

2024-07-21 20 46 47


To test manually:

  • Run python examples/geo_alchemy/app.py
  • Add some points using the create menu. Maps may be blank unless you've setup mapbox, but you can still save points.
  • Go to the list view and load/leave the edit modal a few times.
  • The edit modal should show the map widget, and the list view should only show one map instance throughout.

Repeat this on the master branch and you'll see the modal view is broken.

Do git revert 2fbdabce4c19db0eb144b0aa5049eb0ae7c4db37 and git revert 7df333629faa6baa56d409c507a37882a22c40e9 to manually unwind PR #2407. You'll see the modal view works, but the list view shows more and more elements because global styling is reapplied at the global scope multiple times.

@samuelhwilliams
Copy link
Contributor Author

In the long run we probably want some tests for this. But given it's all interactive/JS-based stuff, that probably means Playwright or similar, and so I'm leaving out of scope for now.

But it's a clear gap in our testing and we should fix it. Will raise an issue...

@hasansezertasan
Copy link
Member

In the long run we probably want some tests for this. But given it's all interactive/JS-based stuff, that probably means Playwright or similar, and so I'm leaving out of scope for now.

But it's a clear gap in our testing and we should fix it. Will raise an issue...

I totally agree with you. I believe playwright should fit our purpose.

Tomorrow, I'll take a look at the changes you made and leave a review.

In #2407 we removed some seemingly-duplicate network requests for
`bs4_modal.js` every time a modal window is loaded.

This would have been fine if that file didn't not include any
side-effects, but it did: it re-applied 'global styling' every time the
JS file is loaded.

The goal here was to process 'styling' for any dynamica/interactive
elements, such as map widgets, and fields that use 'select2' or
'x-editable'.

When we removed these 'extra' JS calls, the modal windows stopped
getting these interactive elements enabled/setup.

This patch restores that functionality, while still retaining the goal
of PR #2407: to prevent duplicate network requests for a file.

We load `bs<x>_modal.js` on all pages that can display modals; this file
now only sets up the event listener that is responsible for making sure
a modal is cleared out/refreshed between multiple modal displays.

The logic for applying styling to the window is now added inline to all
of the relevant modal components.

A further improvement on what we had before is that now, the call to add
styling is scoped specifically to the modal. This prevents the issue
described in #2351, where map elements may show up multiple times on
the page because `applyGlobalStyles` is called multiple times.
Copy link
Member

@hasansezertasan hasansezertasan left a comment

Choose a reason for hiding this comment

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

Since it's not a problem I've encountered before, it seems like I missed it in #2407 😞. There are a lot of moving parts in the frontend code and I have to admit we need to be more careful on these PRs 🤓.

I reviewed the changes you made and everything looks fine to me 🚀.

I also tried to run examples/geo_alchemy/app.py but failed. I can't seem to run the "Setup the database" section properly 🦾.

But I do trust the logic here 🥇. In any case, one more approval might be better.

@hasansezertasan
Copy link
Member

hasansezertasan commented Jul 22, 2024

I also tried to run examples/geo_alchemy/app.py but failed. I can't seem to run the "Setup the database" section properly 🦾.

I don't have to test it with the geo_alchemy example anymore. I tested it with the sqla-association_proxy (used edit_modal=True on UserAdmin) and witnessed the problem.

#2407, prevents the flask_admin/static/admin/js/form.js functionality on modals, causing JS initialized form fields to break - not initialize at all.

I believe we'll need a browser-based testing solution for such problems.

@samuelhwilliams samuelhwilliams merged commit f38df3e into pallets-eco:master Jul 23, 2024
8 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

List view shows each map twice
2 participants