-
Notifications
You must be signed in to change notification settings - Fork 122
Create To Do screen #2809
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
Create To Do screen #2809
Conversation
refs: MBL-17289 affects: Student, Teacher release note: none test plan: See ticket
refs: MBL-17289 affects: Student, Teacher release note: Create ToDo screen in Calendar was updated with new visuals. test plan: See ticket
6320c4c
to
62af9a5
Compare
5e201e3
to
06b65fc
Compare
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.
Went through the code and added some findings, in addition to those we talked about the icons and colors scripts usage in private. Good job in overall, especially on extending the InstUI toolset.
Core/Core/Planner/CreateToDo/ViewModel/CreateToDoViewModel.swift
Outdated
Show resolved
Hide resolved
Core/Core/Planner/CreateToDo/ViewModel/CreateToDoViewModel.swift
Outdated
Show resolved
Hide resolved
Core/Core/Planner/CreateToDo/ViewModel/SelectCalendarViewModel.swift
Outdated
Show resolved
Hide resolved
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 have now a bunch of *Cell components but are they really only used in lists? What I mean is that could they be simply named *View?
When testing for error scenarios I receive a Publishing changes from within view updates is not allowed, this will cause undefined behavior.
on the CreateTodo screen.
public struct LabelValueCell<Label: View>: View { | ||
@Environment(\.dynamicTypeSize) private var dynamicTypeSize | ||
|
||
private let label: Label | ||
private let value: String? | ||
private let action: () -> Void | ||
|
||
public init(label: Label, value: String?, action: @escaping () -> Void) { | ||
self.label = label | ||
self.value = value | ||
self.action = action | ||
} |
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.
Since this component is actually a button, not just a label I think we should either change the name or change the name and make the action parameter optional.
// end of today, to match default web behaviour | ||
date = .now.endOfDay() |
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.
For me this behaviour was strange, I'd expect a picker to work like other mobile app pickers. Idk if we should talk about this with Marci and Judit.
Core/Core/Planner/CreateToDo/ViewModel/CreateToDoViewModel.swift
Outdated
Show resolved
Hide resolved
Core/Core/Planner/CreateToDo/ViewModel/CreateToDoViewModel.swift
Outdated
Show resolved
Hide resolved
Core/Core/Planner/CreateToDo/ViewModel/CreateToDoViewModel.swift
Outdated
Show resolved
Hide resolved
Code Review + 1 for Attila's changes. |
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
refs: MBL-17289
affects: Student, Teacher
release note: Create ToDo screen in Calendar was updated with new visuals.
What's new
BaseScreen
NavigationBarButton
componentInstUI
dynamicTypeSize
environment property in most places (sizeCategory
is deprecated)disabled
andplaceholder
colors to be used where customization is needed/possibleKnown issues
Test plan
Screenshots
Checklist