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 deleteConfirmationDialog #3696

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chore(content-explorer): Migrate deleteConfirmationDialog #3696

wants to merge 1 commit into from

Conversation

greg-in-a-box
Copy link
Contributor

@greg-in-a-box greg-in-a-box commented Oct 4, 2024 •
edited
Loading

before

image

after

image

@greg-in-a-box greg-in-a-box marked this pull request as ready for review October 4, 2024 18:16
@greg-in-a-box greg-in-a-box requested review from a team as code owners October 4, 2024 18:16
tjuanitas
tjuanitas reviewed Oct 7, 2024
View reviewed changes
Copy link
Contributor

@tjuanitas tjuanitas left a comment

Choose a reason for hiding this comment

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

small things

src/elements/content-explorer/DeleteConfirmationDialog.tsx Outdated
<Modal onOpenChange={onCancel} open={isOpen}>
<Modal.Content
aria-label={formatMessage(messages.deleteDialogLabel)}
className="bce-DeleteConfirmationDialog"
Copy link
Contributor

Choose a reason for hiding this comment

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

is this class needed?

src/elements/content-explorer/DeleteConfirmationDialog.tsx Outdated
Comment on lines 20 to 28
isOpen,
onDelete,
onCancel,
item,
isLoading,
parentElement,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: alphasort

src/elements/content-explorer/DeleteConfirmationDialog.tsx Outdated
Comment on lines 51 to 53
</Modal.Footer>
</Modal.Content>
Copy link
Contributor

Choose a reason for hiding this comment

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

the rename dialog also includes a close button, do we need it here?

src/elements/content-explorer/DeleteConfirmationDialog.tsx Show resolved Hide resolved
src/elements/content-explorer/DeleteConfirmationDialog.tsx
Copy link
Contributor

Choose a reason for hiding this comment

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

when the modal is opened, what is the element that is focused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

close is focused, confirmed by hitting enter and a console.log after its open

@greg-in-a-box greg-in-a-box requested a review from a team as a code owner October 7, 2024 20:25
@greg-in-a-box greg-in-a-box force-pushed the deletedialog branch 4 times, most recently from 780e224 to 4598961 Compare November 5, 2024 00:18
greg-in-a-box
greg-in-a-box commented Nov 5, 2024
View reviewed changes
src/elements/common/modal.scss Outdated
@@ -6,6 +8,10 @@
justify-content: center;
padding: 15px 0 0;

[class^='bp_base_button_module_button']:last-child {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

have to put this here until we can use button instead once all the modals have been converted

@greg-in-a-box greg-in-a-box force-pushed the deletedialog branch 2 times, most recently from 92eb0e3 to 3706296 Compare November 5, 2024 02:04
tjuanitas
tjuanitas reviewed Nov 5, 2024
View reviewed changes
src/elements/common/modal.scss Outdated
@@ -6,6 +8,10 @@
justify-content: center;
padding: 15px 0 0;

[class^='bp_base_button_module_button']:last-child {
margin-inline-start: tokens.$space-3;
Copy link
Contributor

Choose a reason for hiding this comment

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

i think now we should move towards using the css vars:

@use '@box/blueprint-web-assets/tokens/tokens-css-vars.scss';
...

.component {
    background-color: var(--background-background);
}

the scss tokens are deprecated in the docs

src/elements/content-explorer/DeleteConfirmationDialog.tsx Outdated
>
{formatMessage(messages.delete)}
</Button>
<Button autoFocus disabled={isLoading} onClick={onCancel} size="large" variant="secondary">
Copy link
Contributor

Choose a reason for hiding this comment

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

is autofocus a property on the blueprint component?

src/elements/content-explorer/DeleteConfirmationDialog.tsx
onRequestClose={onCancel}
overlayClassName={CLASS_MODAL_OVERLAY}
parentSelector={() => parentElement}
portalClassName={`${CLASS_MODAL} be-modal-delete`}
Copy link
Contributor

Choose a reason for hiding this comment

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

is be-modal-delete used?

src/elements/common/modal.scss Outdated
@@ -6,6 +8,10 @@
justify-content: center;
padding: 15px 0 0;

[class^='bp_base_button_module_button']:last-child {
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if we should try using the Modal.Footer from blueprint to handle the buttons

src/elements/content-explorer/stories/DeleteConfirmationDialog.stories.tsx Outdated
@@ -22,27 +18,26 @@ export const deleteDialog = {
setArgs({ isOpen: false });
};

const { appElement, rootElement } = addRootElement();
Copy link
Contributor

Choose a reason for hiding this comment

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

appElement isn't needed even though it's still using react-modal?

@greg-in-a-box greg-in-a-box force-pushed the deletedialog branch 2 times, most recently from 56a8da1 to 2a3127f Compare November 5, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjuanitas tjuanitas tjuanitas left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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