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

add property to override badge style in navigation bar #1977

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

kumarmohit5189
Copy link

@kumarmohit5189 kumarmohit5189 commented Feb 20, 2024

Platforms Impacted

  • iOS
  • visionOS
  • macOS

Description of changes

For NavigationBar currently we don't have a way in case we want to change background color for BadgeLabel. It is picking using NavigationBar theme defined. As part of this change, added capability to override BadgeLableStyle for a particular screen, If this override happen then BadgeLabel will be using updated style for its UIBarButtonItems else it will follow NavigationBar style to set background.

Binary change

Total increase: 5,360 bytes
Total decrease: 0 bytes

File Before After Delta
Total 3,10,97,936 bytes 3,11,03,296 bytes ⚠️ 5,360 bytes
Full breakdown
File Before After Delta
NavigationBar.o 5,49,544 bytes 5,53,480 bytes ⚠️ 3,936 bytes
__.SYMDEF 48,49,960 bytes 48,50,824 bytes ⚠️ 864 bytes
FocusRingView.o 8,21,464 bytes 8,22,024 bytes ⚠️ 560 bytes
### Verification

Validated this change by manual testing with different mode like dark mode / light mode etc. Since this change is using existing UI and flows, so is safe fix.

Visual Verification
Before After

|
Simulator Screenshot - iPhone 15 Pro - 2024-02-20 at 15 24 25 |
Simulator Screenshot - iPhone 15 Pro - 2024-02-20 at 15 27 13
|

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

@anandrajeswaran
Copy link
Contributor

Do we really want to expose this? Good to add an example to the Demo app and that should help with Before/After screenshots to justify why this is a desirable experience

@kumarmohit5189
Copy link
Author

Do we really want to expose this? Good to add an example to the Demo app and that should help with Before/After screenshots to justify why this is a desirable experience

We need this to make Badgelabel style to designed format, without making any change in navigationBar.
Example if we keep navigationBar ar eprimary but want badge to system design (red color).

With primary nav style text and badge is of white color. Please refer attached screenshot.

Simulator Screenshot - iPhone 15 Pro - 2024-02-22 at 10 40 36
Simulator Screenshot - iPhone 15 Pro - 2024-02-22 at 10 46 15

@@ -589,6 +591,12 @@ open class NavigationBar: UINavigationBar, TokenizedControlInternal, TwoLineTitl
titleView.avatarOverrideStyle = style
}

/// Override BadgeLabelStyle for navigation bar
/// - Parameter badgeLabelStyle: updated style to be used
@objc public func overrideBadgeLabelStyle(_ badgeLabelStyle: Style) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This API is unclear and relies on very specific behaviors about Style. That class describes the entire notification bar, not just the label style, so we are making two assumptions:

  1. Callers of this API will understand exactly what will happen to the badge label if they specify a different nav bar style.
  2. We will never change what label styles are associated with each navigation bar style.

If we are going to provide a way to change the color of the label, we should expose the final outcome of this: the button's BadgeLabelStyle, and have people override that.

Copy link
Author

Choose a reason for hiding this comment

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

making that property optional direct not allowed as it seems optional Enum in swift with property marked as @objc not allowed. My thought process here is to use this property only when not nil. Assigning default value will create some confusion.
Screenshot 2024-02-29 at 11 56 23 AM

Copy link
Author

Choose a reason for hiding this comment

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

@mischreiber Have used wrapper class to fix this, please have a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

We definitely don't want to have to create a wrapper class just to expose to objC. One question first - do we actually need to consume this from objC? Assuming we do, still need to go towards the suggestion in this thread of exposing a BadgeLabelStyle rather than a Style

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we need to access this in Objective C class, making it optional (since override happening, user can reset too) we need to add wrapper as Enum optional is not compliable.

Not clear with point - Assuming we do, still need to go towards the suggestion in this thread of exposing a BadgeLabelStyle rather than a Style, can you please explain bit more.

Copy link
Contributor

Choose a reason for hiding this comment

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

Style (exposed to objC with MSFNavigationBarTitleAccessoryStyle) and BadgeLabelStyle (exposed to objC with MSFBadgeLabelStyle) are 2 different types, right?

Copy link
Author

Choose a reason for hiding this comment

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

BadgeLabelButton is created internally in this class. So we won't have access to that object, ideally main app not aware about this thing.
Screenshot 2024-03-05 at 10 29 40 AM

Copy link
Author

Choose a reason for hiding this comment

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

Updated code to override badgeLabelStyle instead of Style. But still we require wrapper to make sure we can set to nil if we don't want override.

@mischreiber
Copy link
Contributor

Additional thought: the UI changes in your "before" and "after" images, but there are no new changes in the Demo app. If the default behavior of the button has changed, that is a major design change and can't be submitted. Otherwise, we should expose a toggle in the navigation controller demo to replicate the change you illustrated.

@kumarmohit5189
Copy link
Author

Additional thought: the UI changes in your "before" and "after" images, but there are no new changes in the Demo app. If the default behavior of the button has changed, that is a major design change and can't be submitted. Otherwise, we should expose a toggle in the navigation controller demo to replicate the change you illustrated.

I wanted to close on #1977 (comment), I will update UI accordingly. Can you please let me know your feedback for my comment? How we should handle this case?

@kumarmohit5189
Copy link
Author

Additional thought: the UI changes in your "before" and "after" images, but there are no new changes in the Demo app. If the default behavior of the button has changed, that is a major design change and can't be submitted. Otherwise, we should expose a toggle in the navigation controller demo to replicate the change you illustrated.

@mischreiber here is UI changes demo, please have a look.

Screen.Recording.2024-03-01.at.9.54.49.AM.mov

@anandrajeswaran
Copy link
Contributor

Do we really want to expose this? Good to add an example to the Demo app and that should help with Before/After screenshots to justify why this is a desirable experience

We need this to make Badgelabel style to designed format, without making any change in navigationBar. Example if we keep navigationBar ar eprimary but want badge to system design (red color).

With primary nav style text and badge is of white color. Please refer attached screenshot.

Simulator Screenshot - iPhone 15 Pro - 2024-02-22 at 10 40 36 Simulator Screenshot - iPhone 15 Pro - 2024-02-22 at 10 46 15

Do we have design input on this - is it really intended that both are possible? Or is one of those the actual design and the other is just a bug?

@kumarmohit5189
Copy link
Author

Do we really want to expose this? Good to add an example to the Demo app and that should help with Before/After screenshots to justify why this is a desirable experience

We need this to make Badgelabel style to designed format, without making any change in navigationBar. Example if we keep navigationBar ar eprimary but want badge to system design (red color).
With primary nav style text and badge is of white color. Please refer attached screenshot.
Simulator Screenshot - iPhone 15 Pro - 2024-02-22 at 10 40 36 Simulator Screenshot - iPhone 15 Pro - 2024-02-22 at 10 46 15

Do we have design input on this - is it really intended that both are possible? Or is one of those the actual design and the other is just a bug?

@anandrajeswaran This is an addition to support unread dot like tab bar item. Generally unread dot should be Red. I raised another PR #1974 for this to keep red dot as separate. But came with suggestion to leverage #1974 (review) badgeLabel itself. Its just we need to expose property.
Please let me know your input for this. Any other better approach we can take here?

@anandrajeswaran
Copy link
Contributor

Do we really want to expose this? Good to add an example to the Demo app and that should help with Before/After screenshots to justify why this is a desirable experience

We need this to make Badgelabel style to designed format, without making any change in navigationBar. Example if we keep navigationBar ar eprimary but want badge to system design (red color).
With primary nav style text and badge is of white color. Please refer attached screenshot.
Simulator Screenshot - iPhone 15 Pro - 2024-02-22 at 10 40 36 Simulator Screenshot - iPhone 15 Pro - 2024-02-22 at 10 46 15

Do we have design input on this - is it really intended that both are possible? Or is one of those the actual design and the other is just a bug?

@anandrajeswaran This is an addition to support unread dot like tab bar item. Generally unread dot should be Red. I raised another PR #1974 for this to keep red dot as separate. But came with suggestion to leverage #1974 (review) badgeLabel itself. Its just we need to expose property. Please let me know your input for this. Any other better approach we can take here?

Let's chat offline with designers as well - presumably there was a reason for this existing design to not be red in this case.

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