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.

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

fix(Drawer): fix incorrect usage of the backdrop #3883

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

plagoa
Copy link
Contributor

@plagoa plagoa commented Dec 7, 2023

I added a story to have a Drawer open on top of some content, namely the GlobalActions that might have some styles clash with the one's on the Drawer. I opted to leave that be checked by Storybook because it might be important to have that tested in case anything changes.

@plagoa plagoa requested a review from a team as a code owner December 7, 2023 10:52
@plagoa plagoa requested review from zettca and MEsteves22 and removed request for a team December 7, 2023 10:52
@github-actions github-actions bot temporarily deployed to uikit-app/pr-3883 December 7, 2023 10:57 Destroyed
@github-actions github-actions bot temporarily deployed to uikit/pr-3883 December 7, 2023 10:57 Destroyed
@github-actions github-actions bot temporarily deployed to uikit-app/pr-3883 December 7, 2023 14:17 Destroyed
@github-actions github-actions bot temporarily deployed to uikit/pr-3883 December 7, 2023 14:17 Destroyed
Copy link
Member

@zettca zettca left a comment

Choose a reason for hiding this comment

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

Nice 👌

There's something wrong with the docs though

packages/core/src/components/Drawer/Drawer.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Drawer/Drawer.stories.tsx Outdated Show resolved Hide resolved
packages/core/src/components/Drawer/Drawer.stories.tsx Outdated Show resolved Hide resolved
@plagoa
Copy link
Contributor Author

plagoa commented Dec 7, 2023

@zettca updated the PR with the recommended changes. Deprecated the showBackdrop property in favor of MUI's own hideBackdrop. Also fixed the no scrolling on the stories.

@github-actions github-actions bot temporarily deployed to uikit/pr-3883 December 7, 2023 16:58 Destroyed
@github-actions github-actions bot temporarily deployed to uikit-app/pr-3883 December 7, 2023 16:58 Destroyed
@github-actions github-actions bot temporarily deployed to uikit/pr-3883 December 7, 2023 17:09 Destroyed
@github-actions github-actions bot temporarily deployed to uikit-app/pr-3883 December 7, 2023 17:09 Destroyed
Copy link
Contributor

@MEsteves22 MEsteves22 left a comment

Choose a reason for hiding this comment

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

Looks good 🙏

I noticed the following:

  • When clicking on the button in the last sample, the page scrolls to the top automatically and the drawer doesn't open. But this happens when we click anywhere on the screen. I don't really understand what's happening but putting everything in one sample helps. So we could add the global action and the button to open the drawer on the "Main" sample. For applitools, we can open the drawer programatically like we do with the dialog component.

  • disableBackdropClick doesn't seem to be working. Suggestion:

const handleOnClose: MuiDrawerProps["onClose"] = (event, reason) => {
    if (reason === "backdropClick" && disableBackdropClick) return;

    onClose?.(event, reason);
  };

  return (
    <MuiDrawer
      (...)
      slotProps={{
        backdrop: {
          (...),
          onClick: (event) => handleOnClose(event, "backdropClick"),
        },
      }}
      onClose={handleOnClose}
      {...others}
    >
      <IconButton
        id={setId(id, "close")}
        className={classes.closeButton}
        variant="secondaryGhost"
        onClick={onClose}
        title={buttonTitle}
      >
        <Close role="none" />
      </IconButton>
      {children}
    </MuiDrawer>
  );

@plagoa
Copy link
Contributor Author

plagoa commented Dec 7, 2023

@MEsteves22 updated the PR. I removed the second story and fixed the disableBackdropClick issue.

@github-actions github-actions bot temporarily deployed to uikit/pr-3883 December 7, 2023 19:36 Destroyed
@github-actions github-actions bot temporarily deployed to uikit-app/pr-3883 December 7, 2023 19:36 Destroyed
MEsteves22
MEsteves22 previously approved these changes Dec 11, 2023
Copy link
Contributor

@MEsteves22 MEsteves22 left a comment

Choose a reason for hiding this comment

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

👌🏻

@MEsteves22 MEsteves22 dismissed their stale review December 11, 2023 09:34

disableBackdropClick

@zettca zettca merged commit 23ccffe into master Dec 11, 2023
7 of 8 checks passed
@zettca zettca deleted the fix/HVUIKIT-6965_drawer branch December 11, 2023 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants