chore(content-explorer): migrate renameDialog #3666

tjiang-box · 2024-09-18T20:51:07Z

Replaced Modal Component by Blueprint Modal

Basic Example

Before

After

Error Example

Before

After

tjiang-box · 2024-09-23T20:41:50Z

scripts/jest/jest-setup.ts

+        dispatchEvent: jest.fn(),
+    })),
+});
+


resolve issue TypeError: window.matchMedia is not a function when running tests
https://jestjs.io/docs/manual-mocks#mocking-methods-which-are-not-implemented-in-jsdom

if i recall this occurs when using fireEvent, can you try using userEvent for tests?

I think he has to use fireEvent.change, I didnt see a way around it when I was doing something similar in the other PR, for everything else he could probably swap out with userEvent

I swap out all fireEvent with userEvents now but still face this error. The matchMedia function might be wrapped internally somewhere.

@greg-in-a-box for fireEvent.change(), we could use userEvent.clear(textInput) + userEvent.type(textInput, 'xxx') to replace.

tjiang-box · 2024-09-23T20:44:47Z

src/elements/common/messages.js

+        id: 'be.webLinkRenameHeading',
+        description: 'Heading for rename web link dialog',
+        defaultMessage: 'Rename Web Link',
+    },


There are 4 types of BoxItem in total ("folder", "file", "hubs", "web_link").

box-ui-elements/src/common/types/core.js

Line 86 in f6abd4e

type ItemType = typeof ITEM_TYPE_FOLDER | typeof ITEM_TYPE_FILE | typeof ITEM_TYPE_HUBS | typeof ITEM_TYPE_WEBLINK;

box-ui-elements/src/common/constants.js

Lines 2 to 5 in f6abd4e

const ITEM_TYPE_FILE: 'file' = 'file';

const ITEM_TYPE_FOLDER: 'folder' = 'folder';

const ITEM_TYPE_HUBS: 'hubs' = 'hubs';

const ITEM_TYPE_WEBLINK: 'web_link' = 'web_link';

are all these flows possible within ContentExplorer? could you check the behavior of this vs EUA?

tjuanitas · 2024-09-24T00:09:00Z

scripts/jest/jest-setup.ts

+        dispatchEvent: jest.fn(),
+    })),
+});
+


if i recall this occurs when using fireEvent, can you try using userEvent for tests?

tjuanitas · 2024-09-24T00:12:18Z

src/elements/content-explorer/RenameDialog.scss

@@ -0,0 +1,5 @@
+@import '../common/variables';


this import isn't used

tjuanitas · 2024-09-24T00:14:42Z

src/elements/content-explorer/RenameDialog.tsx

+    parentElement: HTMLElement;
+}
+
+/* eslint-disable jsx-a11y/label-has-for */


is this still needed? if so, could you use next-line instead of disabling the entire component

good catch! it is not needed since <label> and <input> are replaced by <TextInput>.

tjuanitas · 2024-09-24T00:18:56Z

src/elements/content-explorer/RenameDialog.tsx

+                        {formatMessage(messages.rename)}
+                    </Modal.Footer.PrimaryButton>
+                </Modal.Footer>
+                <Modal.Close aria-label="Close" />


this should use formatMessage

tjuanitas · 2024-09-24T00:20:20Z

src/elements/content-explorer/RenameDialog.scss

+@import '../common/variables';
+
+.be-modal-rename {
+    width: 400px;


is this needed if the size prop is used in Modal.Content?

I checked the max-width for small size is 460px which seems close to what we have before (400 px). I think it is better to remove this style file and set the size to small instead of overriding the width. wdyt @tjuanitas

tjuanitas · 2024-09-24T00:21:16Z

src/elements/common/messages.js

can renameDialogText be removed now?

good question! renameDialogText is still used inside of the renameDialog.js.flow file, so simply removing this message will cause flow check error. I believe those .flow file will be removed eventually after we finish migration? If so, should I leave it here since I believe we will also remove things used in this file such as messages or styles when we do it.

tjuanitas · 2024-09-24T00:23:20Z

src/elements/common/messages.js

@@ -317,6 +317,26 @@ const messages = defineMessages({
        description: 'Error text for rename dialog when name is too long',
        defaultMessage: 'This name is too long.',
    },
+    fileRenameHeading: {


to keep with the pattern of the other ids, maybe renameDialogFileHeader?

that is my initial thought. however the itemType are all set as all lowercase such as file and folder. I was think about two approaches

name message id like above and wrote <FormattedMessage {...messages[${itemType}RenameHeading]} />

name message id like renameDialogFileHeading and wrote <FormattedMessage {...(type === 'file' ? messages.renameDialogFileHeading : messages.renameDialogFolderHeading)} />

update first letter in type to upperCase

I didn't like the second approach because it doesn't give good clarity and readability. Also it lacks extensibility if we want to support more itemTypes in contentExplorer. Nor third approach either because I feel like it is extra implantation just for making messages id aligned. Let me know what you think or any suggestions. @tjuanitas

it's probably just more readable and cleaner to use a switch statement or a map:

const headerMessages = { [ITEM_TYPE_FILE]: ..., [ITEM_TYPE_FOLDER]: ..., [ITEM_TYPE_WEBLINK]: ..., }; <Modal.Header> {formatMessage(headerMessages[type])} </Modal.Header>

or

let header; switch (type) { case "file": header = ... break; case "folder": header = ... break; case "web_link": header = ... break; default: break; }

src/elements/content-explorer/RenameDialog.tsx

tjuanitas · 2024-09-24T00:28:50Z

src/elements/content-explorer/RenameDialog.tsx

+        <Modal onOpenChange={onCancel} open={isOpen}>
+            <Modal.Content
+                aria-label={formatMessage(messages.renameDialogLabel)}
+                className="be-modal-rename"


probably use suit naming: bce-RenameDialog
bce = box-content-explorer

removed this classname since we gonna use size="small"

greg-in-a-box · 2024-09-24T21:56:00Z

src/elements/content-explorer/stories/RenameDialog.stories.js

+import RenameDialog from '../RenameDialog';
+
+// need to import this into the story because it's usually in ContentExplorer
+import '../../common/modal.scss';


do we still need this since we are using blueprint?

nope, ill remove it

greg-in-a-box · 2024-09-24T21:56:23Z

src/elements/content-explorer/stories/RenameDialog.stories.js

+import * as React from 'react';
+import { useArgs } from '@storybook/preview-api';
+
+import PrimaryButton from '../../../components/primary-button/PrimaryButton';


can we replace this with a blueprint button?

sure! sounds good!

greg-in-a-box · 2024-09-24T21:57:22Z

src/elements/content-explorer/stories/RenameDialog.stories.js

can this be a ts/tsx file?

this can be changed to tsx file. if we want to do this, we should update the stories in contentUploader as well.

tjuanitas · 2024-09-24T21:59:31Z

src/elements/common/messages.js

+        description: 'Header for rename web link dialog',
+        defaultMessage: 'Rename Link',
+    },
+    fileRenameHeading: {


this id's are no longer needed anymore since we have the ones above

This reverts commit f5f1bac.

* Revert "chore(content-explorer): migrate renameDialog (#3666)" This reverts commit f5f1bac. * chore(content-explorer): keep changes depended by other elements

* Revert "chore(content-explorer): migrate renameDialog (box#3666)" This reverts commit f5f1bac. * chore(content-explorer): keep changes depended by other elements

tjiang-box requested a review from a team as a code owner September 18, 2024 20:51

tjiang-box force-pushed the migrate_rename_dialog branch 2 times, most recently from b77d9e2 to 5a8960d Compare September 18, 2024 21:11

tjiang-box added the do-not-review label Sep 19, 2024

tjiang-box force-pushed the migrate_rename_dialog branch from 5a8960d to 1bbf408 Compare September 19, 2024 20:44

tjiang-box added 2 commits September 23, 2024 11:39

chore(content-explorer): migrate renameDialog

534c3ba

chore(content-explorer): migrate rename dialog

97c1867

tjiang-box force-pushed the migrate_rename_dialog branch from 1bbf408 to 97c1867 Compare September 23, 2024 20:39

tjiang-box requested review from a team as code owners September 23, 2024 20:39

tjiang-box commented Sep 23, 2024

View reviewed changes

tjiang-box removed the do-not-review label Sep 23, 2024

tjuanitas reviewed Sep 24, 2024

View reviewed changes

tjiang-box added 2 commits September 24, 2024 16:46

chore(content-explorer): update based on comments

a295502

Merge branch 'master' into migrate_rename_dialog

b1ebf48

greg-in-a-box requested changes Sep 24, 2024

View reviewed changes

tjuanitas reviewed Sep 24, 2024

View reviewed changes

tjuanitas previously approved these changes Sep 25, 2024

View reviewed changes

chore(content-explorer): update storybook and messages

2fdf109

tjiang-box dismissed tjuanitas’s stale review via 2fdf109 September 25, 2024 18:09

tjiang-box force-pushed the migrate_rename_dialog branch from 36c5586 to 2fdf109 Compare September 25, 2024 18:09

greg-in-a-box approved these changes Sep 25, 2024

View reviewed changes

tjuanitas approved these changes Sep 25, 2024

View reviewed changes

tjiang-box added the ready-to-merge label Sep 25, 2024

greg-in-a-box merged commit f5f1bac into box:master Sep 25, 2024
4 checks passed

tjiang-box added a commit to tjiang-box/box-ui-elements that referenced this pull request Oct 8, 2024

Revert "chore(content-explorer): migrate renameDialog (box#3666)"

95c6a12

This reverts commit f5f1bac.

tjiang-box mentioned this pull request Oct 8, 2024

revert(content-explorer): Revert rename dialog #3704

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(content-explorer): migrate renameDialog #3666

chore(content-explorer): migrate renameDialog #3666

tjiang-box commented Sep 18, 2024 •

edited by tjuanitas

Loading

tjiang-box Sep 23, 2024 •

edited

Loading

tjuanitas Sep 24, 2024

greg-in-a-box Sep 24, 2024

tjiang-box Sep 24, 2024 •

edited

Loading

tjiang-box Sep 23, 2024

tjuanitas Sep 23, 2024

tjuanitas Sep 24, 2024

tjuanitas Sep 24, 2024

tjuanitas Sep 24, 2024

tjiang-box Sep 24, 2024 •

edited

Loading

tjuanitas Sep 24, 2024

tjuanitas Sep 24, 2024

tjiang-box Sep 24, 2024 •

edited

Loading

tjuanitas Sep 24, 2024

tjiang-box Sep 24, 2024

tjuanitas Sep 24, 2024

tjiang-box Sep 24, 2024

tjuanitas Sep 24, 2024

tjuanitas Sep 24, 2024

tjiang-box Sep 24, 2024

greg-in-a-box Sep 24, 2024

tjiang-box Sep 24, 2024

greg-in-a-box Sep 24, 2024

tjiang-box Sep 24, 2024

greg-in-a-box Sep 24, 2024

tjiang-box Sep 24, 2024

tjuanitas Sep 24, 2024

	const ITEM_TYPE_FILE: 'file' = 'file';
	const ITEM_TYPE_FOLDER: 'folder' = 'folder';
	const ITEM_TYPE_HUBS: 'hubs' = 'hubs';
	const ITEM_TYPE_WEBLINK: 'web_link' = 'web_link';

chore(content-explorer): migrate renameDialog #3666

chore(content-explorer): migrate renameDialog #3666

Conversation

tjiang-box commented Sep 18, 2024 • edited by tjuanitas Loading

tjiang-box Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjiang-box Sep 24, 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

tjiang-box Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjiang-box Sep 24, 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

Choose a reason for hiding this comment

tjiang-box commented Sep 18, 2024 •

edited by tjuanitas

Loading

tjiang-box Sep 23, 2024 •

edited

Loading

tjiang-box Sep 24, 2024 •

edited

Loading

tjiang-box Sep 24, 2024 •

edited

Loading

tjiang-box Sep 24, 2024 •

edited

Loading