Skip to content

Fix Open Button#1450

Merged
demiankatz merged 4 commits intoUniversalViewer:devfrom
Saira-A:fixopenbutton
Jun 16, 2025
Merged

Fix Open Button#1450
demiankatz merged 4 commits intoUniversalViewer:devfrom
Saira-A:fixopenbutton

Conversation

@Saira-A
Copy link
Copy Markdown
Contributor

@Saira-A Saira-A commented Jun 13, 2025

This corrects the open button png to the correct icon.

@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
universalviewer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 16, 2025 11:20am

Copy link
Copy Markdown
Contributor

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

@Saira-A, after thinking about it more, I think I understand the purpose of the settings now: the idea is that you activate this button and set a template pointing at your own instance of the UV. This way, if somebody else embeds your viewer, it has a link "home" to the full-page version. That's why it only displays in the iframe.

I think maybe we should put the iframe check back, but just be sure this is clearly explained in the documentation. In any case, fixing the icon is obviously a good idea, no matter what else we do.

I wonder if it might also be valuable to add a new setting called "simulateIframe" (or similar) that could be used for testing/debugging this -- it could just force isInIFrame to return true. Maybe that's not feasible, though -- I didn't look at the code, so I'm not sure if that function can possibly have access to configs. Maybe something to consider in future if it's not really practical today.

@demiankatz
Copy link
Copy Markdown
Contributor

I've also added this to the community board "in testing" so we remember to discuss during stand-up. :-)

@Saira-A
Copy link
Copy Markdown
Contributor Author

Saira-A commented Jun 13, 2025

@Saira-A, after thinking about it more, I think I understand the purpose of the settings now: the idea is that you activate this button and set a template pointing at your own instance of the UV. This way, if somebody else embeds your viewer, it has a link "home" to the full-page version. That's why it only displays in the iframe.

I think maybe we should put the iframe check back, but just be sure this is clearly explained in the documentation. In any case, fixing the icon is obviously a good idea, no matter what else we do.

I wonder if it might also be valuable to add a new setting called "simulateIframe" (or similar) that could be used for testing/debugging this -- it could just force isInIFrame to return true. Maybe that's not feasible, though -- I didn't look at the code, so I'm not sure if that function can possibly have access to configs. Maybe something to consider in future if it's not really practical today.

Ah, that makes sense. I've put the check back so this PR just fixes the icon for now

demiankatz
demiankatz previously approved these changes Jun 13, 2025
Copy link
Copy Markdown
Contributor

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @Saira-A -- I'll approve, and we can merge on Monday if no one objects (and I can't imagine why they would). :-)

@Saira-A
Copy link
Copy Markdown
Contributor Author

Saira-A commented Jun 13, 2025

Thanks @demiankatz - I did also test setting the config to true and embedding the iframe into a html page and it does work as intended so I've updated the documentation to reflect this

@LanieOkorodudu
Copy link
Copy Markdown
Collaborator

Thanks, @Saira-A -- I'll approve, and we can merge on Monday if no one objects (and I can't imagine why they would). :-)

@LanieOkorodudu
Copy link
Copy Markdown
Collaborator

I agree with your suggestion, @demiankatz to add a simulateIframe option, especially for those who may not be familiar with creating an iframe manually. I created one myself and saved it as an HTML file to test on my end, but as you mentioned, it's definitely something we could consider adding in the future. This looks good to merge! thank you, @Saira-A and @demiankatz !

@demiankatz demiankatz merged commit cf31503 into UniversalViewer:dev Jun 16, 2025
4 checks passed
@github-project-automation github-project-automation Bot moved this from IN TESTING to COMPLETED in DEV EX Community Sprint May-July 2025 Jun 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: COMPLETED

Development

Successfully merging this pull request may close these issues.

3 participants