-
Notifications
You must be signed in to change notification settings - Fork 168
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
base: main
Are you sure you want to change the base?
add property to override badge style in navigation bar #1977
Conversation
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. With primary nav style text and badge is of white color. Please refer attached screenshot. |
@@ -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) { |
There was a problem hiding this comment.
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:
- Callers of this API will understand exactly what will happen to the badge label if they specify a different nav bar style.
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
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? |
…thub.com/kumarmohit5189/fluentui-apple into user/kumarmohit/fluent_bar_button_change
@mischreiber here is UI changes demo, please have a look. Screen.Recording.2024-03-01.at.9.54.49.AM.mov |
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. |
Let's chat offline with designers as well - presumably there was a reason for this existing design to not be red in this case. |
Platforms Impacted
Description of changes
For
NavigationBar
currently we don't have a way in case we want to change background color forBadgeLabel
. It is picking usingNavigationBar
theme defined. As part of this change, added capability to overrideBadgeLableStyle
for a particular screen, If this override happen thenBadgeLabel
will be using updated style for its UIBarButtonItems else it will followNavigationBar
style to set background.Binary change
Total increase: 5,360 bytes
Total decrease: 0 bytes
Full breakdown
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
|
|
|
Pull request checklist
This PR has considered:
Microsoft Reviewers: Open in CodeFlow