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

implement red dot support for UIBarButtonItem #1974

Conversation

kumarmohit5189
Copy link

@kumarmohit5189 kumarmohit5189 commented Feb 15, 2024

Platforms Impacted

  • iOS
  • visionOS
  • macOS

Description of changes

As part of this change, I have added red dot support for UIBarButtonItem. Currently UIBarButtonItem supports only Badge label, to make it similar to TabBarItem in Fluent UI I introduced a property called shouldShowRedDot in similar way as we are using for badge. It will work in similar approach to TabBarItem where:

  • If user set shouldShowRedDot and badgeValue both, it will show badge value and logic will run as it was before change.
  • If user just set shouldShowRedDot, it will display red dot
  • If user just set badgeValue, it will show badge label as it was previously.

Binary change

File Before After Delta
Total 3,10,97,936 bytes 3,11,54,096 bytes 🛑 56,160 bytes
Full breakdown
File Before After Delta
UIBarButtonItem+RedDot.o 0 bytes 25,832 bytes ⚠️ 25,832 bytes
BadgeLabelButton.o 1,09,936 bytes 1,30,520 bytes ⚠️ 20,584 bytes
__.SYMDEF 48,49,960 bytes 48,59,232 bytes ⚠️ 9,272 bytes
FocusRingView.o 8,21,464 bytes 8,21,928 bytes ⚠️ 464 bytes
NavigationBar.o 5,49,544 bytes 5,49,552 bytes ⚠️ 8 bytes

Verification

(how the change was tested, including both manual and automated tests)

Visual Verification

| Before | After |
|-------
Simulator Screenshot - iPhone 15 Pro - 2024-02-15 at 11 44 40
Simulator Screenshot - iPhone 15 Pro - 2024-02-15 at 11 44 42

---------------------------------------|--------------------------------------------|
| |
Simulator Screenshot - iPhone 15 Pro - 2024-02-15 at 11 25 19
Simulator Screenshot - iPhone 15 Pro - 2024-02-15 at 11 25 22
|

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

@kumarmohit5189 kumarmohit5189 requested a review from a team as a code owner February 15, 2024 06:18
Copy link
Contributor

@laminesm laminesm left a comment

Choose a reason for hiding this comment

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

Also, BadgeLabelButton already supports the red dot. All that is needed is to set BadgeLabelStyle to .system and have no text as badgeValue. Though the style and the control are internal, could we just leverage that?
Screenshot 2024-02-15 at 11 47 04 AM

Copy link
Contributor

@laminesm laminesm left a comment

Choose a reason for hiding this comment

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

Also, how do these changes interact with the existing badge styles? Does the red dot override the badge label style?


import UIKit

@objc public extension UIBarButtonItem {
Copy link
Contributor

Choose a reason for hiding this comment

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

We generally do not want to create public extensions to system classes in Fluent, as it forces all downstream consumers to deal with our extension in their namespace. Instead, either use a custom UIBarButtonItem subclass, or provide some form of helper class that can perform the mutation for you.

Copy link
Author

Choose a reason for hiding this comment

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

But we are following same styling for Badge value as well. I am just adding one extra functionality on top of badge button by using same mechanism.

Copy link
Contributor

Choose a reason for hiding this comment

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

The existence of technical debt does not automatically justify the addition of more. We should move the badge styling away from this pattern as well; adding more that needs to be changed will only increase future work, especially when it comes to public APIs that downstream partners expect to remain supported.

updateAccessibilityLabel()
}

@objc private func redDotVisibilitDidChanged() {
Copy link
Contributor

Choose a reason for hiding this comment

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

spelling

@harrieshin
Copy link
Contributor

the concern i have is that this isn't a design paradigm in fluent design for navigation header. the small dot was only used for controls like pillbuttonbar or segmented control so far. why can't you use "1" to represent the unread notification? it follows most apple navigation standards ..

@kumarmohit5189
Copy link
Author

kumarmohit5189 commented Feb 16, 2024

@Harrie

Also, BadgeLabelButton already supports the red dot. All that is needed is to set BadgeLabelStyle to .system and have no text as badgeValue. Though the style and the control are internal, could we just leverage that? Screenshot 2024-02-15 at 11 47 04 AM

Yes, we can do, but from API prospect it looks weird when we set blank title. So, i though to keep clean API so its clearer on what API is exactly does.

Also, about style as System, this is being set in private method itself, so may be confusing while exposing these things.

If still in case we wanted to use BadgeLabel, what is your suggestion for style thing. We can simply add a property similar to badge value? And pick this if set while creating Badge label button.

Please let me know your input here.

@kumarmohit5189
Copy link
Author

the concern i have is that this isn't a design paradigm in fluent design for navigation header. the small dot was only used for controls like pillbuttonbar or segmented control so far. why can't you use "1" to represent the unread notification? it follows most apple navigation standards ..

showing 1 looks weird,

Another option to show red dot is passing blank value, and expose Button style property to App. But passing blank value seems weird here, So I try to make separate path similar to tab bar. Let me know your input for this.

@kumarmohit5189
Copy link
Author

Also, how do these changes interact with the existing badge styles? Does the red dot override the badge label style?

its similar to Tab, If we set Badge and Red dot both, preference will be given to Badge and it will work as it was.


import UIKit

@objc public extension UIBarButtonItem {
Copy link
Contributor

Choose a reason for hiding this comment

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

The existence of technical debt does not automatically justify the addition of more. We should move the badge styling away from this pattern as well; adding more that needs to be changed will only increase future work, especially when it comes to public APIs that downstream partners expect to remain supported.

@laminesm
Copy link
Contributor

@Harrie

Also, BadgeLabelButton already supports the red dot. All that is needed is to set BadgeLabelStyle to .system and have no text as badgeValue. Though the style and the control are internal, could we just leverage that? Screenshot 2024-02-15 at 11 47 04 AM

Yes, we can do, but from API prospect it looks weird when we set blank title. So, i though to keep clean API so its clearer on what API is exactly does.

Also, about style as System, this is being set in private method itself, so may be confusing while exposing these things.

If still in case we wanted to use BadgeLabel, what is your suggestion for style thing. We can simply add a property similar to badge value? And pick this if set while creating Badge label button.

Please let me know your input here.

You can expose the style as a public property through the control that uses the BadgeLabel. For reference, see how we did it here for the tabbar https://github.com/microsoft/fluentui-apple/pull/1818/files

@kumarmohit5189
Copy link
Author

Open another PR as suggested above, please help to review @laminesm @mischreiber @harrieshin

PR link #1977

Closing it.

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.

4 participants