Skip to content

Conversation

carylwyatt
Copy link
Member

@carylwyatt carylwyatt commented Jan 10, 2025

This handy/wild staging utility rewrites links on dev servers to match the subdomain/protocol of the server. When I was removing the cookie banner from pt embeds (DEV-1116), something weird was happening with rewriting the handle URLs for the iframe. Aaron helped me track down this function, and I refactored to make it a bit more robust.

I kept the original if statement but nested it inside another if statement (🫠) just in case it's necessary for something that I am unaware of.

This might not be the best code in the world, but it doesn't affect prod and I needed to add this to stage the work for hathitrust/babel#73

To test: The pt embed iframes are now loading correctly with the minimal UI look. This code isn't staged on the test server yet, so the old function is still causing the iframes on the embed page to use the full UI (navbar, "options" menu, page toolbar), but the dev-3 embed page has the correct "minimal" UI (like prod).

Copy link

@liseli liseli left a comment

Choose a reason for hiding this comment

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

@carylwyatt I reviewed this PR, and the changes to the functions seem fine. Since the old and new versions of the views are available on the test server, I've tested them.

Although the new view looks fine, when I select View Mode to choose between Scroll and Flip. See the image below.

While the new view appears satisfactory, so I've decided to check the View Mode. Selecting the View Mode option allows me to choose between Scroll and Flip modes. Please see the image below.

image

With the old function, I can navigate through the pages of the books; however, with the new function, the loading does not complete. Please see the image below.

image

Is that expected, or is something not to test on this PR?

@carylwyatt
Copy link
Member Author

carylwyatt commented Jan 13, 2025

Thanks for testing, @liseli! The embedded books use the "minimal" UI to try and avoid something like this. If the iframe sets the width of the frame smaller than 700px wide, the view is locked into "Scroll" view. I'm guessing that's why it gives the loading icon and never resolves when switching to "Flip" view: the iframe can't use flip view when the width of the iframe is less than 700px.

@liseli
Copy link

liseli commented Jan 13, 2025

@carylwyatt, Thank you for the detailed explanation. I will approve this PR.

Should we explain to the user why they are locked into the "Scroll" view, or will the view change be restricted?

@carylwyatt
Copy link
Member Author

let's ask @angelinanz and @giramesh -- what do y'all think about removing the "view" button from the embedded viewer (only when the query URL includes ui=embed)? it looks like this and removes the issue where swapping to another view never resolves the loading/spinning waffle icon
image

@giramesh
Copy link

Thanks for tagging @carylwyatt! I think removing the view button makes sense, although I do wonder if it'd be possible to retain the image & plain-text page formats with just the scroll view (i.e. just removing flip and thumbnails) incase it might be useful for users to access that as well. On keyboard tab, one could still access the 'Skip to text-only view' too though.

@carylwyatt
Copy link
Member Author

@giramesh I think that's a good idea! I'll update the code over in the babel repo and tag you there.

@carylwyatt carylwyatt merged commit e304025 into main Jan 15, 2025
6 checks passed
@carylwyatt carylwyatt deleted the DEV-1116 branch January 15, 2025 20:23
@giramesh
Copy link

looks good on all 4 browsers (chrome, firefox, edge and safari), thanks caryl!

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.

3 participants