chore(content-explorer): Migrate previewDialog #3702

greg-in-a-box · 2024-10-07T21:16:01Z

tjuanitas

based on testing, the blueprint modal doesn't seem like a straight replacement for react-modal in this case

tjuanitas · 2024-10-08T02:30:46Z

src/elements/content-explorer/PreviewDialog.tsx

+        <Modal onOpenChange={onCancel} open={isOpen}>
+            <Modal.Content aria-label={formatMessage(messages.preview)} container={parentElement} size="xlarge">
+                <ContentPreview


there's an important behavior change when using the blueprint modal (this applies to the other CE dialogs as well):

the blueprint modal renders on top of the entire webpage while the react-modal was contained to only the container space of the element

for example, if an app has multiple elements on one page (such as ContentPicker and ContentExplorer and maybe some form inputs) then opening preview in a blueprint modal hijacks the entire page instead of rendering over just the ContentExplorer element

this modal also doesn't seem to work well with the dropdown menu or tooltips. have you tested this with sidebar?

i think due to the above issues, it might be worth sticking with react-modal for element dialogs

tjuanitas · 2024-10-08T02:45:36Z

src/elements/content-explorer/PreviewDialog.scss

@@ -0,0 +1,16 @@
+[class^='bp_modal_module_content'].bce-PreviewDialog {


bce-PreviewDialog isn't added to the component

tjuanitas · 2024-10-08T03:23:32Z

src/elements/content-explorer/PreviewDialog.scss

@@ -0,0 +1,16 @@
+[class^='bp_modal_module_content'].bce-PreviewDialog {


we can't rely on class name or specificity as a long term solution

do you have a suggestion in mind about this? VRTs should fail in either case though

tjuanitas · 2024-10-08T03:28:11Z

src/elements/content-explorer/PreviewDialog.scss

+    position: fixed;
+    top: 0;
+    right: 0;
+    z-index: 1000;
+    display: grid;
+    max-width: 100%;
+    max-height: 100%;
+    margin: 0;
+    inset: 0;
+    border-radius: 0;
+
+    .be.bcpr {
+        height: 100vh;
+    }


position: fixed and height: 100vh have same concerns as the other comment about the change in behavior for this modal. these properties will hijack the entire page outside of the element

tjuanitas · 2024-10-08T03:39:55Z

src/elements/content-explorer/PreviewDialog.scss

+    position: fixed;
+    top: 0;
+    right: 0;
+    z-index: 1000;


z-index is pretty fragile, id avoid updating if possible. we'd have to test against how it interacts with other floating components like tooltips, dropdowns, selects, datepicker, flyout, annotations, etc

src/elements/content-explorer/PreviewDialog.js.flow

tjuanitas · 2024-11-04T19:42:38Z

src/elements/content-explorer/PreviewDialog.js.flow

+    onCancel: any,
+    onDownload: any,
+    onPreview: any,
    parentElement: HTMLElement,
    previewLibraryVersion: string,
-    requestInterceptor?: Function,
-    responseInterceptor?: Function,
+    responseInterceptor?: any,
+    requestInterceptor?: any,


Function -> any seems backwards in terms of helpfulness

thats the codemods doing, if i recall correctly

tjuanitas · 2024-11-04T19:43:59Z

src/elements/content-explorer/PreviewDialog.tsx

+        if (onPreview) {
+            onPreview(cloneDeep(data));
+        }


onPreview isn't an optional prop so is this guard necessary?

tjuanitas · 2024-11-04T19:44:32Z

src/elements/content-explorer/PreviewDialog.tsx

+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    onPreview: (data: any) => void;


unknown rather than any? otherwise we should do something about this eslint rule

tjuanitas · 2024-11-04T19:48:20Z

src/elements/content-explorer/PreviewDialog.tsx

+            isOpen={isOpen}
+            parentSelector={() => parentElement}
+            portalClassName={`${CLASS_MODAL} be-modal-preview`}
+            className={CLASS_MODAL_CONTENT_FULL_BLEED}
+            overlayClassName={CLASS_MODAL_OVERLAY}
+            contentLabel={formatMessage(messages.preview)}
+            onRequestClose={onCancel}
+            appElement={appElement}


nit: sorting

tjuanitas · 2024-11-04T19:51:38Z

src/elements/content-explorer/PreviewDialog.js.flow

-type Props = {
+import messages from '../common/messages';
+
+import './PreviewDialog.scss';


does this file exist?

tjuanitas · 2024-11-04T19:53:47Z

src/elements/content-explorer/PreviewDialog.js.flow

-import messages from '../common/messages';
-import ContentPreview from '../content-preview';
+
+import ContentPreview, { ContentPreviewProps } from '../content-preview';


technically i think ContentPreviewProps needs to be imported as a separate type for flow files

tjuanitas · 2024-11-04T19:54:43Z

src/elements/content-explorer/PreviewDialog.tsx

+        <Modal
+            isOpen={isOpen}
+            parentSelector={() => parentElement}
+            portalClassName={`${CLASS_MODAL} be-modal-preview`}


do we need be-modal-preview?

tjuanitas · 2024-11-04T19:57:20Z

src/elements/content-explorer/__tests__/PreviewDialog.test.tsx

+// @ts-ignore
+import APICache from '../../../utils/Cache';


whats the ts error?

tjuanitas · 2024-11-04T20:00:16Z

src/elements/content-explorer/__tests__/PreviewDialog.test.tsx

+
+    test('does not call onPreview with cloned data on load', () => {
+        const data = { id: '1', type: 'file' };
+        renderComponent({ onPreview: null });


when does this scenario happen? onPreview is required in PreviewDialog and has a default prop passed from ContentExplorer

tjuanitas · 2024-11-04T20:24:40Z

src/elements/content-explorer/stories/tests/PreviewDialog-visual.stories.tsx

+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    render: (args: any) => {


we should do something about this too. no point in having the rule if we're just disabling it

greg-in-a-box force-pushed the previewdialog branch 3 times, most recently from 411fa0b to aaa6b3a Compare October 7, 2024 22:50

greg-in-a-box marked this pull request as ready for review October 7, 2024 22:55

greg-in-a-box requested review from a team as code owners October 7, 2024 22:55

greg-in-a-box force-pushed the previewdialog branch from aaa6b3a to 3ec6d98 Compare October 7, 2024 22:55

jpan-box previously approved these changes Oct 7, 2024

View reviewed changes

tjuanitas reviewed Oct 8, 2024

View reviewed changes

greg-in-a-box added do-not-merge do-not-review labels Oct 8, 2024

greg-in-a-box dismissed jpan-box’s stale review via ce6cab1 October 31, 2024 18:12

greg-in-a-box force-pushed the previewdialog branch 5 times, most recently from 28d5e73 to f095b0e Compare November 4, 2024 19:32

greg-in-a-box removed do-not-merge do-not-review labels Nov 4, 2024

jpan-box previously approved these changes Nov 4, 2024

View reviewed changes

greg-in-a-box dismissed jpan-box’s stale review via 5b1fc0a November 4, 2024 19:58

greg-in-a-box force-pushed the previewdialog branch from f095b0e to 5b1fc0a Compare November 4, 2024 19:58

tjuanitas reviewed Nov 4, 2024

View reviewed changes

chore(content-explorer): Migrate previewDialog

ecc3697

greg-in-a-box force-pushed the previewdialog branch from 5b1fc0a to ecc3697 Compare November 4, 2024 20:16

tjuanitas approved these changes Nov 4, 2024

View reviewed changes

jpan-box approved these changes Nov 4, 2024

View reviewed changes

greg-in-a-box added the ready-to-merge label Nov 4, 2024

Merge branch 'master' into previewdialog

e2fdb39

mergify bot merged commit a426f57 into box:master Nov 4, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(content-explorer): Migrate previewDialog #3702

chore(content-explorer): Migrate previewDialog #3702

greg-in-a-box commented Oct 7, 2024

tjuanitas left a comment

tjuanitas Oct 8, 2024

tjuanitas Oct 8, 2024

tjuanitas Oct 8, 2024

tjuanitas Oct 8, 2024

tjuanitas Oct 8, 2024

greg-in-a-box Oct 8, 2024 •

edited

Loading

tjuanitas Oct 8, 2024

tjuanitas Oct 8, 2024

tjuanitas Nov 4, 2024

greg-in-a-box Nov 4, 2024

tjuanitas Nov 4, 2024

tjuanitas Nov 4, 2024

tjuanitas Nov 4, 2024

tjuanitas Nov 4, 2024

tjuanitas Nov 4, 2024

tjuanitas Nov 4, 2024

tjuanitas Nov 4, 2024

tjuanitas Nov 4, 2024

tjuanitas Nov 4, 2024

		@@ -0,0 +1,16 @@
		[class^='bp_modal_module_content'].bce-PreviewDialog {

		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		onPreview: (data: any) => void;

		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		render: (args: any) => {

chore(content-explorer): Migrate previewDialog #3702

chore(content-explorer): Migrate previewDialog #3702

Conversation

greg-in-a-box commented Oct 7, 2024

tjuanitas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-in-a-box Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-in-a-box Oct 8, 2024 •

edited

Loading