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(web-components): refactor fluent-dialog and add new fluent-dialog-body component #31512

Merged
merged 20 commits into from
Jun 4, 2024
Merged

chore(web-components): refactor fluent-dialog and add new fluent-dialog-body component #31512

merged 20 commits into from
Jun 4, 2024

Conversation

davatron5000
Copy link
Contributor

@davatron5000 davatron5000 commented May 29, 2024 •
edited
Loading

Previous Behavior

  • The initial implementation of <fluent-dialog> had a lot of duplicate functionality with native HTMLDialogElement
  • Didn't support content scenarios like a full-width image in the Dialog Body
  • No tests for dialog

New Behavior

  • Cleanup of Dialog component and internal markup
  • Remove unnecessary focus trap behavior
  • Remove open attribute functionality
  • Beef up stories and supported scenarios
  • Rename modalType attribute to type
  • 🆕 Allow for non-padded content like a fulll width image inside dialog
  • 🆕 Add CSS transitions to dialog open/close
  • 🆕 Add <fluent-dialog-body> element to handle default content behavior
  • 🆕 Begin to add tests for <fluent-dialog>

Related Issue(s)

  • Fixes #

@fabricteam
Copy link
Collaborator

fabricteam commented May 29, 2024 •
edited
Loading

📊 Bundle size report

✅ No changes found

@codesandbox-ci CodeSandbox CI
Copy link

codesandbox-ci bot commented May 29, 2024 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@davatron5000 davatron5000 marked this pull request as ready for review May 29, 2024 20:23
@davatron5000 davatron5000 requested a review from a team as a code owner May 29, 2024 20:23
chrisdholt
chrisdholt reviewed May 29, 2024
View reviewed changes
Copy link
Member

@chrisdholt chrisdholt left a comment

Choose a reason for hiding this comment

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

This looks like a great improvement - a couple questions and comments to see if we can trim/simplify/align in a few places

packages/web-components/src/dialog-body/dialog-body.template.ts Outdated Show resolved Hide resolved
packages/web-components/src/dialog-body/dialog-body.template.ts Outdated Show resolved Hide resolved
packages/web-components/src/dialog-body/dialog-body.template.ts Outdated Show resolved Hide resolved
packages/web-components/src/dialog-body/dialog-body.ts Outdated Show resolved Hide resolved
packages/web-components/src/dialog/dialog.styles.ts Show resolved Hide resolved
packages/web-components/src/dialog/dialog.styles.ts Show resolved Hide resolved
packages/web-components/src/dialog/dialog.ts Outdated Show resolved Hide resolved
@davatron5000 davatron5000 force-pushed the davatron5000/refactor-fluent-dialog branch from 2d11400 to f20fe20 Compare May 31, 2024 20:35
radium-v
radium-v reviewed May 31, 2024
View reviewed changes
Copy link
Contributor

@radium-v radium-v left a comment

Choose a reason for hiding this comment

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

The reduction in class methods is incredible to see here, great work!

I'd love to see some more Playwright tests, as well as some duplication cleanup in the story files if that's possible.

packages/web-components/src/dialog-body/dialog-body.stories.ts Outdated Show resolved Hide resolved
packages/web-components/src/dialog-body/dialog-body.styles.ts Show resolved Hide resolved
packages/web-components/src/dialog-body/dialog-body.template.ts Outdated Show resolved Hide resolved
packages/web-components/src/dialog/dialog.spec.ts Outdated Show resolved Hide resolved
packages/web-components/src/dialog/dialog.spec.ts Outdated Show resolved Hide resolved
packages/web-components/src/dialog/dialog.ts Outdated Show resolved Hide resolved
@davatron5000 davatron5000 force-pushed the davatron5000/refactor-fluent-dialog branch from c32edae to d08c0b1 Compare June 3, 2024 22:01
@chrisdholt chrisdholt merged commit 1084cb6 into microsoft:master Jun 4, 2024
19 checks passed
miroslavstastny pushed a commit to miroslavstastny/fluentui that referenced this pull request Jun 14, 2024
@davatron5000 @janechu
@radium-v @miroslavstastny
chore(web-components): refactor fluent-dialog and add new fluent-dial…
4b8653c 
…og-body component (microsoft#31512)

Co-authored-by: Jane Chu <[email protected]>
Co-authored-by: John Kreitlow <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eljefe223 eljefe223 eljefe223 left review comments

@fabricteam fabricteam fabricteam left review comments

@chrisdholt chrisdholt chrisdholt approved these changes

@janechu janechu janechu approved these changes

@radium-v radium-v radium-v approved these changes

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

Successfully merging this pull request may close these issues.
6 participants
@davatron5000 @fabricteam @radium-v @janechu @chrisdholt @eljefe223