-
Notifications
You must be signed in to change notification settings - Fork 122
[MBL-19373] [S] Update to do screen design #3673
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
base: master
Are you sure you want to change the base?
Conversation
refs: MBL-19373 builds: Student affects: Student release note: none test plan: - Compare design with figma. - Check accessibility. - Validate proper grouping by dates.
Affected Apps: StudentBuilds: Student
|
BuildsCommit: Implement code review suggestions. Improve a11y. (92cc847) |
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.
Code +1
// Squeeze height to 0 so day badge goes next to cell. | ||
.frame(height: 0, alignment: .top) |
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.
That's an interesting approach !
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.
Kind of a hack that made this type of header implementation quite easy. The drawback is that when scrolling the next section up, it wont push out the previous section (since its height is 0) but just overlap it.
QA +1 for all UI related features. Though there is one performance issue while scrolling. See below ScreenRecording_10-02-2025.11-10-32_1.MP4 |
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.
Pull Request Overview
This PR introduces a redesigned Todo list screen for the Student app, featuring improved visual design and functionality while maintaining the existing screen as a fallback behind a feature flag. The new design includes grouped todo items by day with sticky headers, enhanced visual elements using shared widget components, and improved course name handling logic.
Key changes:
- Introduced new Todo screen design with sticky day headers and grouped items
- Enhanced course name logic to display nicknames vs. course codes appropriately
- Refactored widget components for reuse between widget and main app screens
- Updated empty state with new vacation panda illustration
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
TodoItemView.swift |
Simplified widget view to use shared content component |
TodoModel.swift |
Updated to use new TodoItemViewModel and enhanced preview data |
TodoListViewModel.swift |
Added day header navigation and empty state configuration |
TodoItemViewModel.swift |
New view model with enhanced course name logic and date formatting |
TodoGroupViewModel.swift |
New view model for day-grouped todo items |
TodoListScreen.swift |
Redesigned screen with sticky headers and grouped layout |
TodoItemContentView.swift |
New shared component for consistent todo item display |
TodoDayHeaderView.swift |
New sticky header component for day grouping |
Course.swift |
Added originalName property and nickname detection |
APICourse.swift |
Added original_name field to API model |
Various test files | Updated tests to work with new view models and data structures |
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.
QA + 1
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.
QA +1
The scrolling issue now is fixed 👍
ScreenRecording_10-13-2025.11-21-07_1.MP4
# Conflicts: # Core/CoreTests/Features/Todos/Model/TodoInteractorLiveTests.swift
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.
Thanks for this work, nice solution for the day-headers!
Overall it looks good to me, I have some a11y comments, and some minor code-related comments (some of which are about spreading the word about existing helpers in the codebase :) )
|
||
override func setUp() { | ||
super.setUp() | ||
Clock.mockNow(Self.mockDate) |
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.
Please add Clock.reset()
in teardown
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.
I didn't add because it's in the CoreTestCase
's setUp
method. Maybe also adding it to the tearDown
would be appropriate.
.multilineTextAlignment(.leading) | ||
} | ||
|
||
private var contextSection: some View { |
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 could InstUI.JoinedSubtitleLabels
instead of the HStack.
It has a divider which matches figma.
(I would say center aligning the icon looks better, but that's doable either way)
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.
Done.
let dateComponents = DateComponents(year: 2021, month: 8, day: 7, hour: 12) | ||
let date = Calendar.current.date(from: dateComponents)! |
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.
Suggestion: Date.make()
(same for the others)
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.
Good idea!
|
||
let group = TodoGroupViewModel(date: date, items: items) | ||
|
||
XCTAssertTrue(group.accessibilityLabel.contains("Saturday")) |
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.
Suggestion: XCTAssertContains
(same for the others)
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.
self.htmlURL = plannable.htmlURL | ||
|
||
self.color = plannable.color.asColor | ||
self.icon = Image(uiImage: plannable.icon) |
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.
self.icon = Image(uiImage: plannable.icon) | |
self.icon = plannable.icon.asImage |
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.
Done.
// Check second group (tomorrow) | ||
let secondGroup = todoGroups[1] | ||
XCTAssertEqual(secondGroup.items.count, 1) | ||
XCTAssertEqual(secondGroup.items[0].title, "Quiz 1") |
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.
suggestion: using first/last instead of [0]/[1] to ensure the test doesn't crash, just fails (even though CI ignores the crash, it stops execution locally)
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.
// When | ||
interactor.refreshResult = .success(false) | ||
// When - with non-empty todos | ||
interactor.refreshResult = .success(()) |
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.
interactor.refreshResult = .success(()) | |
interactor.refreshResult = .success |
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.
Done.
.foregroundStyle(item.color) | ||
.accessibilityHidden(true) | ||
.frame(maxHeight: .infinity, alignment: .top) | ||
.padding(.top, isCompactLayout ? 0 : 2) |
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.
Why do we need this extra padding?
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.
The widget and the in-app todo rows use different font heights and for a one liner todo title this offset gave the best vertical centering effect. I modified the logic to scale this with font scale, now the text is centered with the first line of the text most of the time.
} | ||
|
||
private var titleSection: some View { | ||
VStack(alignment: .leading) { |
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 should have the same spacing as the main vstack here, which is zero at the moment.
(It is usually 2, maybe figma does not have this by mistake)
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.
Right, the huge spacing felt strange but since I copied I guessed that's the correct. I modified the logic to have 0 spacing for the widget (compact layout) and 2 points for the in-app cell.
@rh12 I implemented your suggestions, now the screen properly displays even at the largest a11y size 🥳 |
What's new?
refs: MBL-19373
builds: Student
affects: Student
release note: none
test plan:
new_student_todo_screen
feature flag.Screenshots
Checklist