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

Jump to bottom

chore(content-explorer): Migrate renameDialog #3740

Merged
merged 1 commit into from
Nov 20, 2024
Merged

chore(content-explorer): Migrate renameDialog #3740

merged 1 commit into from
Nov 20, 2024

Conversation

greg-in-a-box
Copy link
Contributor

@greg-in-a-box greg-in-a-box commented Nov 12, 2024 •
edited
Loading

image

jpan-box
jpan-box previously approved these changes Nov 12, 2024
View reviewed changes
@greg-in-a-box greg-in-a-box marked this pull request as ready for review November 12, 2024 18:41
@greg-in-a-box greg-in-a-box requested review from a team as code owners November 12, 2024 18:41
@greg-in-a-box greg-in-a-box force-pushed the renamedialog branch 3 times, most recently from c1cf36d to c17bc31 Compare November 14, 2024 18:17
jpan-box
jpan-box previously approved these changes Nov 14, 2024
View reviewed changes
tjuanitas
tjuanitas reviewed Nov 15, 2024
View reviewed changes
src/elements/content-explorer/__tests__/RenameDialog.test.tsx Outdated

test('calls onCancel when cancel button is clicked', async () => {
renderComponent();
await userEvent.click(await screen.findByRole('button', { name: 'Cancel' }));
Copy link
Contributor

Choose a reason for hiding this comment

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

a lot of these seem to use find but why not use get? the buttons should appear on initial render and then the await isn't needed. that way there's no confusion about whether there's a delay or something in the rendering of this component

src/elements/content-explorer/__tests__/RenameDialog.test.tsx Outdated
Comment on lines 73 to 74
expect(defaultProps.onRename).not.toHaveBeenCalled();
expect(defaultProps.onCancel).toHaveBeenCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

i dont think we should check a shared object like this. these tests are passing because they're relying on global jest setting clearMocks: true but i don't think we should rely on that and it could set up bad habits when developers work in other repos.

creating local variables like this makes the tests more independent and intentional:

test('...', async () => {
    const onCancel = jest.fn();
    const onRename = jest.fn();

    renderComponent({ onCancel, onRename });
    await userEvent.click(screen.getByText('Rename'));
    expect(onRename).not.toHaveBeenCalled();
    expect(onCancel).toHaveBeenCalled();
});

src/elements/content-explorer/__tests__/RenameDialog.test.tsx Outdated
expect(defaultProps.onRename).toHaveBeenCalledWith('newname', '.txt');
});

test('displays an error message when errorCode is ERROR_CODE_ITEM_NAME_IN_USE', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should not be ERROR_CODE_ITEM_NAME_IN_USE

src/elements/content-explorer/__tests__/RenameDialog.test.tsx Outdated
Comment on lines 55 to 63
test('displays an error message when errorCode is ERROR_CODE_ITEM_NAME_IN_USE', () => {
renderComponent({ errorCode: ERROR_CODE_ITEM_NAME_IN_USE });
expect(screen.getByText('An item with the same name already exists.')).toBeInTheDocument();
});

test('displays an error message when errorCode is ERROR_CODE_ITEM_NAME_IN_USE', () => {
renderComponent({ errorCode: ERROR_CODE_ITEM_NAME_IN_USE });
expect(screen.getByText('An item with the same name already exists.')).toBeInTheDocument();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

duplicate tests

src/elements/content-explorer/RenameDialog.tsx Outdated
Comment on lines 107 to 113
<Text as="label">
{error && (
<div className="be-modal-error">
<FormattedMessage {...error} values={{ name: nameWithoutExt }} />
</div>
)}
</Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

if there's no error then this renders an empty label right? should error be moved out a level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing in favor for the below comment

src/elements/content-explorer/RenameDialog.tsx Outdated
Comment on lines 114 to 117
<Text as="label">
<FormattedMessage {...messages.renameDialogText} values={{ name: nameWithoutExt }} />
</Text>
<input ref={ref} defaultValue={nameWithoutExt} onKeyDown={onKeyDown} required type="text" />
Copy link
Contributor

Choose a reason for hiding this comment

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

what about using TextInput from blueprint to handle label, input, and error?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tjiang-box had a PR to redesign the modal to align more with EUA: #3666 i think this was confirmed with design

@greg-in-a-box greg-in-a-box force-pushed the renamedialog branch 2 times, most recently from c07954f to d15b910 Compare November 15, 2024 20:57
tjuanitas
tjuanitas reviewed Nov 18, 2024
View reviewed changes
src/elements/content-explorer/RenameDialog.js.flow Outdated
import Button from '../../components/button/Button';
import messages from '../common/messages';
import { FormattedMessage, useIntl } from 'react-intl';
import {Modal as BlueprintModal, TextInput} from '@box/blueprint-web';
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: looks like this file might need formatting?

src/elements/content-explorer/RenameDialog.js.flow Outdated
Comment on lines 109 to 115
error={
error && (
<div className="be-modal-error">
<FormattedMessage {...error} values={{ name: nameWithoutExt }} />
</div>
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think we need these divs anymore?

error={error && formatMessage(error, { name: nameWithoutExt })}

src/elements/common/modal.scss Outdated
.be-modal-share .be-modal-dialog-content,
.be-modal-delete .be-modal-dialog-content {
padding: 0;
border-radius: tokens.$radius-4;
}

.be-modal-rename .be-modal-dialog-content {
input {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about using input[type='text'] so doesn't target something like buttons?

tjuanitas
tjuanitas previously approved these changes Nov 20, 2024
View reviewed changes
tjuanitas
tjuanitas reviewed Nov 20, 2024
View reviewed changes
src/elements/content-explorer/__tests__/RenameDialog.test.tsx Outdated
const onCancel = jest.fn();

renderComponent({ onCancel });
await userEvent.click(await screen.getByRole('button', { name: 'Cancel' }));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i don't think await on getByRole is needed. only for findByRole

Copy link
Contributor

Choose a reason for hiding this comment

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

same for some other cases below

@greg-in-a-box greg-in-a-box merged commit e967f6d into box:master Nov 20, 2024
4 checks passed
@greg-in-a-box greg-in-a-box deleted the renamedialog branch November 20, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjuanitas tjuanitas tjuanitas approved these changes

@jpan-box jpan-box jpan-box approved these changes

Assignees
No one assigned
Labels
ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@greg-in-a-box @tjuanitas @jpan-box