Skip to content

[FluentNotification] Enable showing an action button and a dismiss button#2154

Merged
nodes11 merged 8 commits intomicrosoft:mainfrom
nodes11:notification_dismiss_default
May 20, 2025
Merged

[FluentNotification] Enable showing an action button and a dismiss button#2154
nodes11 merged 8 commits intomicrosoft:mainfrom
nodes11:notification_dismiss_default

Conversation

@nodes11
Copy link
Copy Markdown
Contributor

@nodes11 nodes11 commented May 12, 2025 •
edited
Loading

Platforms Impacted

  • iOS
  • visionOS
  • macOS

Description of changes

There is a use case where we need to be able to show an action button and a dismiss button on a single notification. Both show iff the MSFNotificationState.showDefaultDismissActionButton is true, actionButtonAction is not nil,and actionButtonTitle is not nil and not empty.

To implement this:

  • Add a defaultDimissButtonAction block property on the MSFNotificationState. If the value is not set by the consumer, we will give it a default value in the MSFNotification initializer.
  • Create a dismissButton viewbuilder in the view implementation. This button will only show when the conditions in the first paragraph above are met.
  • Add a call to add the dimiss button if the dismissButtonAction is non-nil. The code to determine the dismissButtonAction was broken out into its own private function so that it could be more easily called from two places in the view.

Binary change

(how is our binary size impacted -- see https://github.com/microsoft/fluentui-apple/wiki/Size-Comparison)

Verification

  • Validated the behavior in the Fluent Test App (see demo)
Visual Verification
Screen.Recording.2025-05-14.at.10.44.55.AM.mov

Pull request checklist

This PR has considered:

  • Light and Dark appearances
  • iOS supported versions (all major versions greater than or equal current target deployment version)
  • VoiceOver and Keyboard Accessibility
  • Internationalization and Right to Left layouts
  • Different resolutions (1x, 2x, 3x)
  • Size classes and window sizes (iPhone vs iPad, notched devices, multitasking, different window sizes, etc)
  • iPad Pointer interaction
  • SwiftUI consumption (validation or new demo scenarios needed)
  • Objective-C exposure (provide it only if needed)
Microsoft Reviewers: Open in CodeFlow

@nodes11 nodes11 marked this pull request as ready for review May 14, 2025 17:53
@nodes11 nodes11 requested a review from a team as a code owner May 14, 2025 17:53
@joannaquu
Copy link
Copy Markdown
Contributor

joannaquu commented May 14, 2025

@huwilkes do you remember the history behind why there's this complicated logic of if we show dismiss button, action button, which one gets linked to which action, etc? why was it not just declarative with action button, dismiss button

@huwilkes
Copy link
Copy Markdown
Collaborator

huwilkes commented May 14, 2025

@huwilkes do you remember the history behind why there's this complicated logic of if we show dismiss button, action button, which one gets linked to which action, etc? why was it not just declarative with action button, dismiss button

The design doesn't have an option with two buttons.

@cbowdoin cbowdoin self-requested a review May 14, 2025 23:04
@cbowdoin cbowdoin assigned cbowdoin and unassigned cbowdoin May 14, 2025
@mischreiber
Copy link
Copy Markdown
Collaborator

mischreiber commented May 15, 2025 •
edited
Loading

@huwilkes do you remember the history behind why there's this complicated logic of if we show dismiss button, action button, which one gets linked to which action, etc? why was it not just declarative with action button, dismiss button

The design doesn't have an option with two buttons.

IIRC most of the API logic was inherited from the previous maintainers of this codebase. But yeah like @huwilkes said, it was a safe assumption to say "if we have an action then show a button, and if we have a dismissal show an ×".

@nodes11 nodes11 force-pushed the notification_dismiss_default branch from fdc3723 to 5b02252 Compare May 15, 2025 23:12
@nodes11
Copy link
Copy Markdown
Contributor Author

nodes11 commented May 16, 2025

The latest commit introduces the property showActionButtonAndDismissButton, which determines whether an action button and a dismiss button can be displayed simultaneously on a notification. Without this property, and with the new behavior applied, all toast notifications that do not explicitly set showDefaultDismissActionButton to true would start showing a dismiss icon. This would be an undesirable and unexpected UX change for clients. To opt into the new behavior—where both an action button and a dismiss button can be displayed at once—clients will need to explicitly set showActionButtonAndDismissButton to true.

mischreiber
mischreiber approved these changes May 19, 2025
View reviewed changes
@nodes11 nodes11 force-pushed the notification_dismiss_default branch from eda4651 to 8944682 Compare May 20, 2025 02:04
@nodes11 nodes11 enabled auto-merge (squash) May 20, 2025 02:05
@nodes11 nodes11 merged commit bc5972d into microsoft:main May 20, 2025
7 checks passed
@mischreiber mischreiber mentioned this pull request Jun 13, 2025
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joannaquu joannaquu joannaquu left review comments

@mischreiber mischreiber mischreiber approved these changes

@cbowdoin cbowdoin cbowdoin 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.

5 participants

@nodes11 @joannaquu @huwilkes @mischreiber @cbowdoin