Skip to content

H-4322: Allow fullscreening image entities; graph viz/slide fixes #6885

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

Merged
merged 5 commits into from
Apr 15, 2025

Conversation

CiaranMn
Copy link
Member

@CiaranMn CiaranMn commented Apr 8, 2025

🌟 What is the purpose of this PR?

  • Add a 'full screen' button when looking at an image file entity, to allow fullscreening the image
  • Graph viz bug fixes
    • Handle edge scaling being set to geometric when all edges are the same weight
    • Make edge sizing max work
  • Move some comments that weren't quite in the right place and were thus confusing

Pre-Merge Checklist 🚀

🚢 Has this modified a publishable library?

This PR:

  • does not modify any publishable blocks or libraries, or modifications do not need publishing

📜 Does this require a change to the docs?

The changes in this PR:

  • are internal and do not require a docs change

🕸️ Does this require a change to the Turbo Graph?

The changes in this PR:

  • do not affect the execution graph

@github-actions github-actions bot added area/apps > hash* Affects HASH (a `hash-*` app) area/libs Relates to first-party libraries/crates/packages (area) type/eng > frontend Owned by the @frontend team area/apps labels Apr 8, 2025
@CiaranMn CiaranMn requested a review from vilkinsons April 15, 2025 08:14
@CiaranMn CiaranMn marked this pull request as ready for review April 15, 2025 08:14
@vilkinsons vilkinsons requested a review from hashdotai April 15, 2025 08:30
Copy link
Member

@hashdotai hashdotai left a comment

Choose a reason for hiding this comment

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

This PR introduces the ability to fullscreen image entities and includes several graph visualization fixes. The changes are generally well-implemented with a few areas that could be improved.

The fullscreen functionality looks solid, though there are some minor accessibility considerations with the button implementation. The fixes for graph visualization, particularly the edge scaling when all edges have the same weight and making edge sizing max work, are well-thought-out. I like that you've added checks for the edge case where edgeSignificanceRatio === 1, which would cause mathematical issues with the logarithm.

The code comment relocations make sense and help clarify the purpose of certain blocks of code. The fallback behavior in draw-chip-with-icon.ts silently handles image loading failures, which works but could benefit from a warning message to aid debugging.

Overall, this PR implements the required functionality well while fixing the issues described in the ticket, but some small improvements could be made to the code quality and maintainability. None of these issues are critical enough to block merging.

vilkinsons
vilkinsons previously approved these changes Apr 15, 2025
Copy link
Member

@vilkinsons vilkinsons left a comment

Choose a reason for hiding this comment

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

@CiaranMn Some AI reviewer comments to take a look at, but nothing from my side here. Thanks for sorting.

@CiaranMn CiaranMn added this pull request to the merge queue Apr 15, 2025
Merged via the queue into main with commit 8d66b18 Apr 15, 2025
35 checks passed
@CiaranMn CiaranMn deleted the cm/full-screen-image-files branch April 15, 2025 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/apps > hash* Affects HASH (a `hash-*` app) area/apps area/libs Relates to first-party libraries/crates/packages (area) type/eng > frontend Owned by the @frontend team
Development

Successfully merging this pull request may close these issues.

3 participants