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

Update Admin Panel to work via iframes #799

Merged
merged 61 commits into from
Feb 4, 2025
Merged

Update Admin Panel to work via iframes #799

merged 61 commits into from
Feb 4, 2025

Conversation

WillNilges
Copy link
Collaborator

@WillNilges WillNilges commented Jan 4, 2025

Refactors the way the map is incorporated into the admin panel. Instead of embedding it and doing a lot of tricks to update the DOM, it puts the admin panel and the map into their own iframes, and ties them together with a little JavaScript secret sauce.

  • Adds ability to pick up where you left off. Your browser will remember the last page you visited and bring you back there if you go to /admin

Still a lot to do on this:

  • Capture forward, back, refresh, etc to make sure people don't lose their place
  • Resize + Hide the map
  • Mobile
  • Refactoring
  • Tests?


# TODO (wdn): Add more tests checking if navigating to xyz page works
# Unfortunately, because that is a lot of javascript, it's tricky to test.
# It may be possible to run selenium integration tests or something to validate
Copy link
Member

Choose a reason for hiding this comment

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

I'd support selenium-based tests for integration testing deployed copies of the application (dev, gamma, etc) but probably we don't want it in our unit testing pipeline, those things are slow enough already

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably not in our unit testing pipeline, no. Are you OK with me handling this in a separate PR or would you like me to take a crack at a solution for this here?

@Andrew-Dickinson
Copy link
Member

Doing some local testing and noticed a few issues:

  • After clicking "logout" in the header, the map is still shown until refreshing the page
  • Clicking "View Site" in the header results in a weird page where the map is shown next to the landing page
  • LOSes and APs don't seem to show on the map when you select them in the left panel

Overall it feels much snappier though, great job on the performance improvements

@WillNilges
Copy link
Collaborator Author

WillNilges commented Jan 21, 2025

After clicking "logout" in the header, the map is still shown until refreshing the page

That's odd. My last commit should have fixed that. Did you pull the latest?

Clicking "View Site" in the header results in a weird page where the map is shown next to the landing page

Fixed

LOSes and APs don't seem to show on the map when you select them in the left panel

Should be fixed

Copy link
Member

@Andrew-Dickinson Andrew-Dickinson left a comment

Choose a reason for hiding this comment

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

I'm not looking super closely, but I don't want to block you any more than I already have. You've iterated on this a ton and I trust your judgement about the readiness of this for release



@staff_member_required
def admin_iframe_view(request: HttpRequest) -> HttpResponse:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Might want to add a test for this authentication/redirect

if (url.pathname.startsWith("/admin/logout")) return true;
if (url.pathname.endsWith("/export/") && isForm) return true;
if (url.host !== location.host) return true;
console.log(`Admin Panel src set to: ${admin_panel_iframe.src}`);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Might want to gate all these log lines with a debug statement or something for cleanliness.

@WillNilges WillNilges merged commit 813083f into main Feb 4, 2025
12 checks passed
@WillNilges WillNilges self-assigned this Mar 8, 2025
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.

2 participants