-
Notifications
You must be signed in to change notification settings - Fork 89
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
Conversation
There was a problem hiding this 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.
apps/hash-frontend/src/pages/shared/entity/entity-editor/file-preview-section.tsx
Show resolved
Hide resolved
apps/hash-frontend/src/pages/shared/entity/entity-editor/file-preview-section.tsx
Show resolved
Hide resolved
...sh-frontend/src/pages/shared/graph-visualizer/graph-container/shared/simple-autocomplete.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
🌟 What is the purpose of this PR?
Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR: