-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix(ui): open links from inside modal in a new tab #14431
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
fix(ui): open links from inside modal in a new tab #14431
Conversation
✅ Meticulous spotted 0 visual differences across 1458 screens tested: view results. Meticulous evaluated ~10 hours of user flows against your PR. Expected differences? Click here. Last updated for commit 5399412. This comment will update as new commits are pushed. |
Bundle ReportChanges will increase total bundle size by 9.8kB (0.03%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
i think this is nice, but we could even add it to our component library potentially!
if there are issues with that then we can go forward with this for now and add it to more places later
<ModuleDetailsForm form={form} formValues={{ name: currentName }} /> | ||
<AssetsSection selectedAssetUrns={selectedAssetUrns} setSelectedAssetUrns={setSelectedAssetUrns} /> | ||
</ModalContent> | ||
<ModalContext.Provider value={{ isInsideModal: true }}> |
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.
even better, let's just move this to our Modal
component we have in our component library so then we know we'll have this context in every modal! what do you think?
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.
cool!
Linear ticket:
https://linear.app/acryl-data/issue/CH-599/fix-opening-of-a-modal-of-a-module-after-redirecting-with-an-opened
Description:
This PR creates a generic
ModalContext
and hookuseGetModalLinkProps
to get link props that determine that a link be opened in a new tab if it's inside a modal.This is used in Asset Collection modal on the homepage. It can be used at any other place by wrapping the modal contents with this ModalContext.