Skip to content
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.

Sign up for GitHub

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

Portal toasters into app provider + Fix toaster hrefs to include path prefix. #16696

Closed
wants to merge 6 commits into from
Closed

Portal toasters into app provider + Fix toaster hrefs to include path prefix. #16696

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
093ed66
portal
salazarm Sep 21, 2023
3767250
layout effect
salazarm Sep 21, 2023
de2e167
async
salazarm Sep 21, 2023
9d41d5d
portal elements + fix href on toasters
salazarm Sep 21, 2023
aacd20a
comment
salazarm Sep 21, 2023
68a197b
rm double slash
salazarm Sep 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion js_modules/dagster-ui/packages/ui-components/src/components/Toaster.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {createGlobalStyle} from 'styled-components';

import {Colors} from './Colors';
import {IconName, Icon} from './Icon';
import {createToaster} from './createToaster';
import {createToaster, getBaseHref} from './createToaster';

export const GlobalToasterStyle = createGlobalStyle`
.dagster-toaster {
Expand Down Expand Up @@ -71,6 +71,9 @@ const setup = (instance: ToasterInstance): DToaster => {
</>
);
}
if (rest?.action?.href) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an interesting fix but I am worried this behavior is inconsistent with the rest of the UI kit -- if you make an <a href= or a window.open(href) call, you have to pass the correct href. Why should toaster let you pass an incorrect href and fix it for you?

We have some places like RunUtils.tsx where we already pass the href correctly built from history.createHref. Can you update the callsites that are doing it incorrectly to do it right instead?

I'm worried that these places that ARE doing it right will have the href "double prepended" with the path prefix now.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alternatively, we could extend the Toaster data to let you specify action: {text: string, to: string} and then have the toaster transform the "to" into an href. I think that'd be consistent with how <Link works and would be nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have some places like RunUtils.tsx where we already pass the href correctly built from history.createHref. Can you update the callsites that are doing it incorrectly to do it right instead?

Yeah I was considering doing that too but the utilities creating the href aren't hooks so they can't access createHref. I guess I can update their callsites to pass the history object from useHistory to them. We could also check if it already has the prefix and if so skip adding it.

rest.action.href = getBaseHref() + rest.action.href;
}
return show.apply(instance, [rest, key]);
};

Expand Down
24 changes: 21 additions & 3 deletions js_modules/dagster-ui/packages/ui-components/src/components/createToaster.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,32 @@
// eslint-disable-next-line no-restricted-imports
import {IToasterProps, Toaster} from '@blueprintjs/core';
import * as React from 'react';
import {createRoot} from 'react-dom/client';

type PortalProvider = (node: React.ReactNode, container: HTMLElement, key?: string) => void;
const queue: Parameters<PortalProvider>[] = [];
let _portalProvider: PortalProvider = (node, container, key) => {
queue.push([node, container, key]);
};

let _baseHref = '';

export const registerPortalProvider = (portalProvider: PortalProvider, baseHref: string) => {
salazarm marked this conversation as resolved.
Show resolved Hide resolved
_baseHref = baseHref;
while (queue.length) {
portalProvider(...queue.pop()!);
}
_portalProvider = portalProvider;
};

export const getBaseHref = () => _baseHref;

// https://github.com/palantir/blueprint/issues/5212#issuecomment-1318397270
export const createToaster = (props?: IToasterProps, container = document.body) => {
const containerElement = document.createElement('div');
container.appendChild(containerElement);
const root = createRoot(containerElement);

return new Promise<Toaster>((resolve, reject) => {
root.render(
_portalProvider(
<Toaster
{...props}
usePortal={false}
Expand All @@ -21,6 +38,7 @@ export const createToaster = (props?: IToasterProps, container = document.body)
}
}}
/>,
containerElement,
);
});
};
2 changes: 2 additions & 0 deletions js_modules/dagster-ui/packages/ui-core/src/app/AppProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {JobFeatureProvider} from '../pipelines/JobFeatureContext';
import {WorkspaceProvider} from '../workspace/WorkspaceContext';

import {AppContext} from './AppContext';
import {AppToasterPortalProvider} from './AppToasterPortalProvider';
import {CustomAlertProvider} from './CustomAlertProvider';
import {CustomConfirmationProvider} from './CustomConfirmationProvider';
import {LayoutProvider} from './LayoutProvider';
Expand Down Expand Up @@ -207,6 +208,7 @@ export const AppProvider: React.FC<AppProviderProps> = (props) => {
<AnalyticsContext.Provider value={analytics}>
<InstancePageContext.Provider value={instancePageValue}>
<JobFeatureProvider>
<AppToasterPortalProvider />
<LayoutProvider>{props.children}</LayoutProvider>
</JobFeatureProvider>
</InstancePageContext.Provider>
Expand Down
28 changes: 28 additions & 0 deletions js_modules/dagster-ui/packages/ui-core/src/app/AppToasterPortalProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import {registerPortalProvider} from '@dagster-io/ui-components/src/components/createToaster';
import React from 'react';
import ReactDOM from 'react-dom';
import {useHref} from 'react-router-dom-v5-compat';

// A hook that makes toaster elements render inside of our React router dom hook and that fixes their hrefs
// to take --path-prefix into account
export const AppToasterPortalProvider = () => {
const [portaledElements, setPortalElements] = React.useState<
[React.ReactNode, HTMLElement, string | undefined][]
>([]);
const baseHref = useHref('/');
React.useLayoutEffect(() => {
registerPortalProvider((node, container, key) => {
setPortalElements((elements) => {
return [...elements, [node, container, key]];
});
}, baseHref);
}, [baseHref]);

return (
<>
{portaledElements.map(([node, container, key], index) =>
ReactDOM.createPortal(node, container, key ?? String(index)),
)}
</>
);
};