[MBL-3054] Fix liquid glass styling of disabled nav bar buttons#2782
Conversation
| .onReceive(self.reactiveViewModel.resetEditableText) { newValue in | ||
| self.showLoading = !newValue | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code is just copy-pasted from the main view builder above. The only change is in the lines below
There was a problem hiding this comment.
That's a nice little bit of cleanup!
| |> UIButton.lens.titleLabel.font .~ UIFont.ksr_body() | ||
| |> UIButton.lens.titleColor(for: .normal) .~ LegacyColors.ksr_create_700.uiColor() | ||
| |> UIButton.lens.titleColor(for: .disabled) .~ LegacyColors.ksr_support_300.uiColor() | ||
|
|
There was a problem hiding this comment.
I first tried to keep the styling in this method and add the glass effect here. Interestingly enough, bindStyles gets called enough times/at weird times that setting the button configuration here causes the title to disappear 🤷 so now this styling code gets called only once, during set up.
amy-at-kickstarter
left a comment
There was a problem hiding this comment.
These changes LGTM. I have a couple non-blocking suggestions for making these changes a little more self-documenting, and adding a little extra testing.
| .onReceive(self.reactiveViewModel.resetEditableText) { newValue in | ||
| self.showLoading = !newValue | ||
| } | ||
| } |
There was a problem hiding this comment.
That's a nice little bit of cleanup!
| @@ -35,6 +35,9 @@ final class ChangePasswordViewController: UIViewController, MessageBannerViewCon | |||
| self.saveButtonView.addTarget(self, action: #selector(self.saveButtonTapped(_:))) | |||
|
|
|||
| let navigationBarButton = UIBarButtonItem(customView: self.saveButtonView) | |||
There was a problem hiding this comment.
Is there a pragmatic reason we have all of these UIButtons wrapped in UIBarButtonItems? Could we implement the same loading spinner behavior in a plain UIBarButtonItem?
Cleaning that up seems out-of-scope for this fix, but I think it might be worth throwing in a little explanation in the code - just so we know why we had to do this, and so we can clean it up if we need to.
I'd suggest something like adding a well-documented exetension, like
extension UIBarButtonItem {
//We have to do this because ...
convenience init(button: UIButton) {
self.init(customView: button)
if #available(iOS 26.0, *) {
navigationBarButton.hidesSharedBackground = true
}
}
}There was a problem hiding this comment.
Hmm, that's a good idea. I added this as a static helper function to the LoadingBarButtonItemView instead, because I think that's the most relevant context for explaining what's going on here.
Getting rid of the UIButton entirely is an interesting idea. I do think that would get us a slightly more polished result (the frame height of the liquid glass effect looks like it's slightly shorter when the effect is applied to the button directly). I do think implementing the loading state could get a bit complicated, since we don't want users to be able to tap the button when the loading indicator is showing, but I also don't think we want the loading indicator to look grayed out. I'm going to stick with the UIButton for now, but I think we should explore this idea if we do need to make more changes here.
| @@ -3,34 +3,61 @@ import Library | |||
| import Prelude | |||
There was a problem hiding this comment.
Is LoadingBarButtonItemView covered in any meaningful screenshot test? Would it be a huge pain to give it its own screenshot test? Just thinking it would be nice to verify there were no regressions with deleting the nib file.
There was a problem hiding this comment.
Good call! I've added snapshot tests. I did mess with the commit history so I could add the snapshot tests before making changes to the view itself.
9e481fb to
0880c1d
Compare
| loadingBarButtonItemView.setTitle(title: "Button") | ||
| loadingBarButtonItemView.setIsEnabled(isEnabled: false) | ||
|
|
||
| assertSnapshot(of: loadingBarButtonItemView, as: .image) |
There was a problem hiding this comment.
I considered using our test helpers instead, but they do not respect intrinsic content size width, so I went with this approach instead. I'm only testing this in light mode because a) this view is old enough that it has not been designed/optimized for dark mode and b) because the background is transparent, dark mode tests look off.
📲 What
Remove the liquid glass styling from the container nav bar views and add it directly to the contained button. This makes the liquid glass effect respect when the button is in a disabled state.
I've tried to keep the iOS 18 styling mostly the same, but I had to get rid of the loading bar button storyboard to get the glass effect to behave nicely, so I'm including before/after comparisons of both iOS 18 and iOS 26.
👀 See
Jira
✅ Acceptance criteria
Todo
ChangeEmailViewController- this is no longer in use after we built a swiftUI version.