Skip to content

Conversation

suhaibabsi-inst
Copy link
Contributor

@suhaibabsi-inst suhaibabsi-inst commented Dec 10, 2024

refs: MBL-18169
affects: Student
release note: Fixed appearance (light/dark mode) issues for Course Smart Search

Test Plan

See ticket description.

Checklist

  • Follow-up e2e test ticket created
  • A11y checked
  • Tested on phone
  • Tested on tablet
  • Tested in dark mode
  • Tested in light mode
  • Approve from product

refs: MBL-18169
affects: Student
release note: Fixed appearance (light/dark mode) issues for Course Smart Search
@suhaibabsi-inst suhaibabsi-inst self-assigned this Dec 10, 2024
@suhaibabsi-inst suhaibabsi-inst changed the title Fix Appearance Issues of Course Smart Search Fix Appearance (Light/Dark Mode) Issues of Course Smart Search Dec 10, 2024
@inst-danger
Copy link
Contributor

inst-danger commented Dec 10, 2024

Student Build QR Code:

@inst-danger
Copy link
Contributor

inst-danger commented Dec 10, 2024

Fails
🚫 Build failed, skipping coverage check

Release Note:

Fixed appearance (light/dark mode) issues for Course Smart Search

Affected Apps: Student

MBL-18169

❌ XCTest failed: CoreTests/AssignmentCellViewModelTests/testSubmissionStatusAndIconAndColor
XCTAssertEqual failed: ("#6a7883") is not equal to ("#697783")
XCTAssertEqual failed: ("#03893d") is not equal to ("#03893c")

Generated by 🚫 dangerJS against 17f96a2

@suhaibabsi-inst suhaibabsi-inst marked this pull request as ready for review December 10, 2024 10:42
Copy link
Collaborator

@vargaat vargaat 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 on functionality. I left a comment about the color lookup helpers that I would change.

Comment on lines 102 to 106
var color: UIColor? { contextColor?.color }

var contextColor: ContextColor? {
contextColor(in: AppEnvironment.shared.database.viewContext)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would remove these helpers to make it explicit that these properties are not part of the Context object and fast to access because in the background they do a CoreData fetch that could have negative effect on performance if used frequently for example during SwiftUI rendering. The functions below are clearer for me that they work with CoreData to produce the result and might be slow.

Copy link
Collaborator

@vargaat vargaat 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

Copy link
Contributor

@rh12 rh12 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

@rh12 rh12 merged commit ff979dd into master Dec 16, 2024
2 of 4 checks passed
@rh12 rh12 deleted the bugfix/MBL-18169-SmartSearch-Appearance-Issues branch December 16, 2024 16:47
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.

4 participants