feat: Dates Screen Stylistic Changes #253

shafqat-muneer · 2024-01-23T06:56:57Z

Jira Tickets:

LEARNER-9735: iOS - Dates Screen Stylistic Changes
LEARNER-9739: iOS - Icons beside course date links on the dates screen

Github Issues:

[iOS] Dates Tab Stylistic Changes #194
[iOS] Icon beside course content on the dates screen #196
[iOS] UI issue in Dark mode on "Dates" tab #241 (This issue is now obsolete, as we have implemented a new design for the dates module and have successfully addressed the dark mode within it.)

Light Mode

Simulator.Screen.Recording.-.iPhone.15.-.2024-01-23.at.11.21.07.mp4

Dark Mode

Simulator.Screen.Recording.-.iPhone.15.-.2024-01-23.at.11.21.47.mp4

saeedbashir · 2024-01-24T04:34:18Z

Core/Core/Assets.xcassets/Date/Contents.json

+{
+  "info" : {
+    "author" : "xcode",
+    "version" : 1
+  }
+}


How about renaming the Date to CourseDates for clarity?

saeedbashir · 2024-01-24T04:38:05Z

Core/Core/Extensions/DateExtension.swift

+        }
+    }
+
+    private func applyDateFormatterForShortWeekdayMonthDayYear(dateFormatter: DateFormatter) {


How about renaming it to applyShortWeekdayMonthDayYear as dateFormatter is a parameter so I guess there isn't any need to mention it in the name.

saeedbashir · 2024-01-24T04:39:45Z

Core/Core/Extensions/DateExtension.swift

+            case -6...(-2):
+                return dateFormatterString
+            case 2...6:
+                return self.timeAgoDisplay()


self is redundant.

saeedbashir · 2024-01-24T04:39:56Z

Core/Core/Extensions/DateExtension.swift

+                    return dateFormatterString
+                } else {
+                    // It means, date is in hours past due or upcoming
+                    return self.timeAgoDisplay()


self is redundant.

saeedbashir · 2024-01-24T04:40:32Z

Core/Core/Extensions/DateExtension.swift

+    }
+
+    func isCurrentYear() -> Bool {
+        let years = Calendar.current.dateComponents([.year], from: self, to: Date())


How about renaming years to datecomponents or to components?

Renamed it to components as it seems more appropriate.

saeedbashir · 2024-01-24T05:10:07Z

Course/Course/Domain/Model/CourseDates.swift

-
-        let calendar = Calendar.current
-        let todayComponents = calendar.dateComponents([.year, .month, .day], from: .today)
+    var sortedStatusToDateToCourseDateBlockDict: [CompletionStatus: [Date: [CourseDateBlock]]] {


How about giving it a simpler name like statusDatesBlocks or sortedStatusDatesBlocks thoughts?

Names now simplified

saeedbashir · 2024-01-24T05:12:54Z

Course/Course/Domain/Model/CourseDates.swift

+        var statusToDateToCourseDateBlockDict: [CompletionStatus: [Date: [CourseDateBlock]]] = [:]
+        var statusToCourseDateBlockDict: [CompletionStatus: [CourseDateBlock]] = [:]


They are local variables so simpler names will be good.

Names now simplified

saeedbashir · 2024-01-24T05:13:36Z

Course/Course/Domain/Model/CourseDates.swift

-
-            if dateComponents == todayComponents {
-                hasToday = true
+            switch true {


Any reason to have switch on true?

It's just an alternate approach to prevent having a lengthy series of if-else blocks. Each case independently assesses its own condition to build a completion status dictionary.

saeedbashir · 2024-01-24T05:24:24Z

Theme/Theme/Assets.xcassets/Colors/CardView/CardViewBackground.colorset/Contents.json

+          "blue" : "0.984",
+          "green" : "0.980",
+          "red" : "0.976"


Any reason to change the CardViewBackground color?

We offer expandable/collapsible sections labeled as "Completed." These sections come with their own background and border stroke colors for both dark and light modes. The designer provided these colors in Figma designs, which closely resemble our existing color palette. Instead of introducing a new set of colors, I made slight adjustments to the existing palette to ensure its versatility.

For instance, in light mode, the background color of the "Completed" section is now CCD4E0, whereas it was previously FFFFFF (white). This change was made to address the issue of indistinguishability between the screen background and the section background, using the specified Figma color for clarity.

Light Mode Dark Mode

For safer side, I introduced separate colors for calendar completed section background and stroke colors in light and dark mode.

saeedbashir · 2024-01-24T05:24:38Z

Theme/Theme/Assets.xcassets/Colors/CardView/CardViewStroke.colorset/Contents.json

@@ -23,7 +23,7 @@
        "color-space" : "srgb",
        "components" : {
          "alpha" : "1.000",
-          "blue" : "0.435",
+          "blue" : "0.439",


Any reason to change this color?

Reason is same as described here.

saeedbashir

LGTM 👍

rnr

Good work! just small comments in code

rnr · 2024-01-26T17:17:04Z

Course/Course/Presentation/Dates/CourseDatesView.swift

+                    ForEach(Array(viewModel.sortedStatuses), id: \.self) { status in
+                        let courseDateBlockDict = courseDates.statusDatesBlocks[status]!
+                        if status == .completed {
+                            CompletedBlocks(isExpanded: $isExpanded,


could you please change this to 'each parameter on own line' style?

rnr · 2024-01-26T17:17:32Z

Course/Course/Presentation/Dates/CourseDatesView.swift

+                                        Text(block.formattedDate)
+                                            .font(Theme.Fonts.labelMedium)
+                                            .foregroundStyle(Theme.Colors.textPrimary)
+                                        BlockStatusView(viewModel: viewModel,


the same here

rnr · 2024-01-26T17:24:19Z

Course/Course/Presentation/Dates/CourseDatesView.swift

+                                    Image(systemName: "chevron.right")
+                                        .resizable()
+                                        .scaledToFit()
+                                        .frame(width: 6.55, height: 11.15)


Just curious, how do you calculate that kind of accuracy? :)

I got this precision from figma designs 😄

rnr · 2024-01-26T17:26:22Z

Course/Course/Presentation/Dates/CourseDatesViewModel.swift

-    func blocks(for date: Date) -> [CourseDateBlock] {
-        courseDates?.sortedDateToCourseDateBlockDict[date] ?? []
+    var sortedStatuses: [CompletionStatus] {
+        let desiredSequence = [CompletionStatus.completed,


'each parameter on own line' style please

rnr · 2024-01-26T17:34:11Z

Course/Course/Data/CourseRepository.swift

@@ -230,7 +230,7 @@ class CourseRepositoryMock: CourseRepositoryProtocol {

    func getCourseDates(courseID: String) async throws -> CourseDates {
        do {
-            let courseDates = try courseDatesJSON.data(using: .utf8)!.mapResponse(DataLayer.CourseDates.self)
+            let courseDates = try CourseRepository.courseDatesJSON.data(using: .utf8)!.mapResponse(DataLayer.CourseDates.self)


do we need // swiftlint:disable all above if you moved json string out this file?

Thanks for pointing it out. Just removed and refactored some warnings.

rnr · 2024-01-26T17:36:49Z

I think this should say '1 item' instead of 'items'

rnr · 2024-01-26T17:38:11Z

This text looks too small and it's grey also - hard to read this.

@moiz994 what do you think?

rnr · 2024-01-26T17:41:09Z

@shafqat-muneer @moiz994
Perhaps we need to follow a uniform arrows animation style for expandable areas (we have one in coursework already)?
Thank you

Screen.Recording.2024-01-26.at.17.56.49.mov

Screen.Recording.2024-01-26.at.17.57.09.mov

shafqat-muneer · 2024-01-29T10:43:35Z

This text looks too small and it's grey also - hard to read this. @moiz994 what do you think?

In the Figma designs, the font size is set to 11px, but our application doesn't support this specific size. Our options are either 10 or 12. I opted for Theme.Fonts.labelSmall, which has a size of 10. If we want to increase it to 12, we can choose Theme.Fonts.labelMedium. By selecting Theme.Fonts.labelMedium, it will resemble the screenshot provided.

On the matter of the color grey, I utilized the same color as in Figma for the light mode, as there is no specified dark mode color for the description text. Therefore, the light mode color was exactly same as of ThisWeekTimeline bar color. For the dark mode, I used the ThisWeekTimeline bar dark mode provided color.

rnr · 2024-01-29T11:03:56Z

This text looks too small and it's grey also - hard to read this. @moiz994 what do you think?

In the Figma designs, the font size is set to 11px, but our application doesn't support this specific size. Our options are either 10 or 12. I opted for Theme.Fonts.labelSmall, which has a size of 10. If we want to increase it to 12, we can choose Theme.Fonts.labelMedium. By selecting Theme.Fonts.labelMedium, it will resemble the screenshot provided.

On the matter of the color grey, I utilized the same color as in Figma for the light mode, as there is no specified dark mode color for the description text. Therefore, the light mode color was exactly same as of ThisWeekTimeline bar color. For the dark mode, I used the ThisWeekTimeline bar dark mode provided color.

@shafqat-muneer
I think you can create another font in the theme if needed for design purposes (I believe we shouldn't be limited 10 and 12 only)
For example, the font labelMediumSmall its size 11.
Thank you

shafqat-muneer · 2024-01-29T11:19:29Z

@shafqat-muneer @moiz994 Perhaps we need to follow a uniform arrows animation style for expandable areas (we have one in coursework already)? Thank you

Screen.Recording.2024-01-26.at.17.56.49.mov

Screen.Recording.2024-01-26.at.17.57.09.mov

Done. It will now look like this:
https://github.com/openedx/openedx-app-ios/assets/11990137/f0112edd-8c81-4e37-bd7b-930983cd88a0

shafqat-muneer · 2024-01-30T05:03:47Z

This text looks too small and it's grey also - hard to read this. @moiz994 what do you think?

In the Figma designs, the font size is set to 11px, but our application doesn't support this specific size. Our options are either 10 or 12. I opted for Theme.Fonts.labelSmall, which has a size of 10. If we want to increase it to 12, we can choose Theme.Fonts.labelMedium. By selecting Theme.Fonts.labelMedium, it will resemble the screenshot provided.
On the matter of the color grey, I utilized the same color as in Figma for the light mode, as there is no specified dark mode color for the description text. Therefore, the light mode color was exactly same as of ThisWeekTimeline bar color. For the dark mode, I used the ThisWeekTimeline bar dark mode provided color.

@shafqat-muneer I think you can create another font in the theme if needed for design purposes (I believe we shouldn't be limited 10 and 12 only) For example, the font labelMediumSmall its size 11. Thank you

@rnr We don't need to introduce a new size. I verified this with Sam here, and he recommended using the labelMedium style (font size 12). Therefore, I adjusted it from 10 to 12.

shafqat-muneer · 2024-01-30T05:08:36Z

@rnr All feedback has been addressed. Kindly review. Thank you.

shafqat-muneer requested a review from saeedbashir January 23, 2024 06:56

feat: Dates Screen Stylistic Changes

b02e13b

shafqat-muneer force-pushed the Shafqat/LEARNER-9735-DateScreenChanges branch from 42112fd to b02e13b Compare January 23, 2024 10:37

shafqat-muneer mentioned this pull request Jan 23, 2024

[iOS] UI issue in Dark mode on "Dates" tab #241

Closed

saeedbashir requested changes Jan 24, 2024

View reviewed changes

shafqat-muneer added 3 commits January 25, 2024 12:33

refactor: address review feedback

1899706

refactor: address review feedback

c5967f6

refactor: address review feedback

0656c15

shafqat-muneer requested review from rnr and volodymyr-chekyrta January 25, 2024 18:23

This was linked to issues Jan 25, 2024

[iOS] Dates Tab Stylistic Changes #194

Closed

[iOS] Icon beside course content on the dates screen #196

Open

saeedbashir previously approved these changes Jan 26, 2024

View reviewed changes

rnr reviewed Jan 26, 2024

View reviewed changes

refactor: address review feedback

77f5b89

shafqat-muneer dismissed saeedbashir’s stale review via 77f5b89 January 29, 2024 11:14

refactor: address review feedback

070c1ce

volodymyr-chekyrta approved these changes Jan 30, 2024

View reviewed changes

rnr approved these changes Jan 30, 2024

View reviewed changes

shafqat-muneer merged commit f4a19e0 into openedx:develop Jan 30, 2024
3 checks passed

shafqat-muneer deleted the Shafqat/LEARNER-9735-DateScreenChanges branch January 30, 2024 13:06

		var statusToDateToCourseDateBlockDict: [CompletionStatus: [Date: [CourseDateBlock]]] = [:]
		var statusToCourseDateBlockDict: [CompletionStatus: [CourseDateBlock]] = [:]

feat: Dates Screen Stylistic Changes #253

feat: Dates Screen Stylistic Changes #253

Conversation

shafqat-muneer commented Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saeedbashir left a comment

Choose a reason for hiding this comment

rnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnr commented Jan 26, 2024

rnr commented Jan 26, 2024

rnr commented Jan 26, 2024 • edited Loading

shafqat-muneer commented Jan 29, 2024

rnr commented Jan 29, 2024

shafqat-muneer commented Jan 29, 2024

shafqat-muneer commented Jan 30, 2024

shafqat-muneer commented Jan 30, 2024

shafqat-muneer commented Jan 23, 2024 •

edited

Loading

rnr commented Jan 26, 2024 •

edited

Loading