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(dialog): ensure enableBackgroundUI is passed to Modal #7167

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Conversation

DobroTora
Copy link
Contributor

@DobroTora DobroTora commented Jan 21, 2025

fix #6023

adding modal props to Dialog and pass enableBackgroundUI

Proposed behaviour

Current behaviour

Screenshot 2025-01-21 at 13 06 28

Checklist

  • Commits follow our style guide
  • Related issues linked in commit messages if required
  • Screenshots are included in the PR if useful
  • All themes are supported if required
  • Unit tests added or updated if required
  • Playwright automation tests added or updated if required
  • Storybook added or updated if required
  • Translations added or updated (including creating or amending translation keys table in storybook) if required
  • Typescript d.ts file added or updated if required
  • Related docs have been updated if required

QA

  • Tested in provided StackBlitz sandbox/Storybook
  • Add new Playwright test coverage if required
  • Carbon implementation matches Design System/designs
  • UI Tests GitHub check reviewed if required

Additional context

Testing instructions

@DobroTora DobroTora requested review from a team as code owners January 21, 2025 13:05
@DobroTora DobroTora marked this pull request as draft January 22, 2025 09:35
@DobroTora DobroTora self-assigned this Jan 22, 2025
src/components/dialog/dialog.component.tsx Outdated Show resolved Hide resolved
src/components/dialog/dialog.component.tsx Outdated Show resolved Hide resolved
src/components/dialog/dialog.component.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@tomdavies73 tomdavies73 left a comment

Choose a reason for hiding this comment

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

I think we're almost there, the spreading rest approach does work, and enableBackgroundUI will be passed down to the modal component. I'd just have a look at my other comments, as we can probably remove changes in 5 files total, and keep the changes limited to dialog.

I think adding some sort of test to verify the prop works, and create a baseline for the changes made would be beneficial. I'd take a look at the first playwright test in the sidebar pw file 👍

src/components/action-popover/action-popover.stories.tsx Outdated Show resolved Hide resolved
src/components/dialog-full-screen/components.test-pw.tsx Outdated Show resolved Hide resolved
src/components/modal/modal.test.tsx Outdated Show resolved Hide resolved
@DobroTora DobroTora force-pushed the FE-5889 branch 4 times, most recently from 3ffbec5 to b1f1791 Compare January 28, 2025 10:33
@DobroTora DobroTora changed the title fix(Modal, Dialog, Alert): adding modal props to Dialog and pass enableBackgroundUI fix(dialog): ensure enableBackgroundUI is passed to Modal Jan 28, 2025
Copy link
Contributor

@tomdavies73 tomdavies73 left a comment

Choose a reason for hiding this comment

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

Good stuff @DobroTora,

Happy to approve when the tests are moved into the correct describe block 👍

src/components/dialog/dialog.pw.tsx Outdated Show resolved Hide resolved
tomdavies73
tomdavies73 previously approved these changes Jan 28, 2025
@DobroTora DobroTora marked this pull request as ready for review January 31, 2025 10:01
stephenogorman
stephenogorman previously approved these changes Jan 31, 2025
@DobroTora DobroTora requested a review from Parsium January 31, 2025 16:08
@Parsium Parsium removed their request for review January 31, 2025 16:13
@Parsium Parsium merged commit 6c11d63 into master Jan 31, 2025
28 checks passed
@Parsium Parsium deleted the FE-5889 branch January 31, 2025 16:14
@carbonci
Copy link
Collaborator

🎉 This PR is included in version 147.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Dialog does not pass enableBackgroundUI to Modal
6 participants