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

Navigation bar titles #3129

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

rh12
Copy link
Contributor

@rh12 rh12 commented Feb 11, 2025

refs: MBL-18460
affects: Student, Teacher, Parent
release note: none

What's changed

  • Unified navigation bar title/subtitle SwiftUI implementations
    • into InstUI.NavigationBarTitleView, it's used via navigationBarTitleView()
    • it works in SwiftUI NavigationView too, not just in UINavigationController
    • it supports VoiceOver properly
    • Updated all SwiftUI screens to use this one
  • Kept UIKit solution TitleSubtitleView, but now it is only used in ViewControllers
  • Updated title font to semibold16 and subtitle font to regular14

How to set navigation bar title/subtitle

  • call navigationBarTitleView(_:) or navigationBarTitleView(title:, subtitle:) modifier
  • AFTERWARDS call navigationBarStyle(_:) modifier
  • don't use the native navigationTitle() methods

Chores

  • removed deprecated method calls
  • removed unused Inbox AttachmentPicker
  • removed leftover .modalLight navigation bar style
  • move related files under CommonUI/NavigationBar folder

Test Plan

  • Verify the following screens' navigation title view reads out "[title], [subtitle], Heading" in VoiceOver
    • Student: Assignment Details screen
    • Student: Grade List Preferences screen
    • Parent: Grade List Preferences screen
  • Verify CourseDetails screen nav bar works properly together with Smart Search
  • Verify various kind of screens open properly in Modules (or at least as close to properly as before)
  • Smoke test nav bars on some screens (see "Files changed" tab)

Checklist

  • A11y checked
  • Tested on phone @ iOS 18
  • Tested on phone @ iOS 17
  • Tested on tablet @ iOS 18
  • Tested on tablet @ iOS 17
  • Tested in dark mode
  • Tested in light mode
  • Approve from product

@inst-danger
Copy link
Contributor

inst-danger commented Feb 11, 2025

Teacher Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Feb 11, 2025

Parent Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Feb 11, 2025

Student Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Feb 11, 2025

Warnings
⚠️ This pull request will not generate a release note.

Affected Apps: Student, Teacher, Parent

MBL-18460

Coverage New % Master % Delta
Canvas iOS 91.68% 91.52% 0.16%

Generated by 🚫 dangerJS against bc5726c

@rh12 rh12 force-pushed the bugfix/MBL-18460-NavigationBar-titles branch from d1268aa to bc5726c Compare February 11, 2025 13:11
/// - Parameters:
/// - title: The line is always displayed, even if this is empty. (This should not happen normally.)
/// - subtitle: The subtitle line is only displayed if this is not empty.
public func navigationBarTitleView(title: String, subtitle: String?) -> some View {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is considered as being an implicit dependency and can be easily missed out.
Rather than dictating that a specific modifier must be called after this one for it work properly.
I would go with passing style as a required parameter to this modifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it's not ideal, and I've considered passing style here, but it creates it's own problems.
Style sets the background color as well, and as a next step it's planned to govern the NavigationBarItems' color (via navBarColors.tint). That would mean we would have to set the style in 3 separate places which is also not ideal, plus it allows for mixed styles which we want to restrict.

I consider this solution as a lesser evil. And since order already matters for SwiftUI modifiers, it's a known concept (although maybe in an unexpected way, yes). Another mitigation of the issue is that this solution (and only this) would be used all over the project, with plenty of examples and documentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure I conveyed my idea correctly. What I mean with passing style, as simple as having style modifier inside this one.

    public func navigationBarTitleView(title: String, subtitle: String?, style: NavigationBarStyle) -> some View {
        toolbar {
            ToolbarItem(placement: .principal) {
                InstUI.NavigationBarTitleView(title: title, subtitle: subtitle)
            }
        }
        .navigationBarStyle(style)
    }

You mean even doing that can create problems ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, basically I aim to avoid this:

content
    .navigationBarTitleView(title: ..., style: .modal)
    .navBarItems(trailing: ..., style: .modal)
    .navigationBarStyle(.modal)

or even:

content
    .navigationBarTitleView(title: ..., style: .modal)
    .navBarItems(trailing: ..., style: .modal) // which would call .navigationBarStyle() inside again in your example

Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst left a comment

Choose a reason for hiding this comment

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

Code +1

@@ -30,7 +30,7 @@ public struct CalendarToDoDetailsScreen: View {
InstUI.BaseScreen(state: viewModel.state, config: viewModel.screenConfig) { _ in
eventContent
}
.navigationTitle(viewModel.navigationTitle)
.navigationBarTitleView(viewModel.navigationTitle)
Copy link
Contributor

Choose a reason for hiding this comment

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

No navigationBarStyle() is called after this.

Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst left a comment

Choose a reason for hiding this comment

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

QA +1

Tested on iPhone, iOS 18.2.0, Student and Parent apps.

One note though, I noticed that Title View on Assignments Screen takes a split second before appearing. See record below:

ScreenRecording_02-12-2025.17-34-00_1.MP4

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.

3 participants