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

All Announcements View #175

Merged
merged 6 commits into from
Jan 20, 2025
Merged

All Announcements View #175

merged 6 commits into from
Jan 20, 2025

Conversation

rahulon12
Copy link
Member

@rahulon12 rahulon12 commented Jan 19, 2025

Fixes --

Changes Made

  • Introduce All Announcements View
  • Use a new APIRequest to fetch announcements across courses using contextCodes.
  • Expose Announcement Row as a public struct and reuse it in the new view.
  • Fixes issue where announcements may show HTML entities (like &nbsp).
  • Fixes issue where courses reload upon every navigation back to the course list (Sidebar.swift)
  • Performance Optimizations can be done further.

Screenshots (if applicable)

image

@rahulon12 rahulon12 marked this pull request as ready for review January 19, 2025 19:05
@rahulon12 rahulon12 marked this pull request as draft January 19, 2025 19:07
@rahulon12 rahulon12 removed the request for review from azooz2003-bit January 19, 2025 19:07
@rahulon12 rahulon12 marked this pull request as ready for review January 19, 2025 19:18
@rahulon12 rahulon12 force-pushed the rahul/all_announcements branch from 7084325 to 2a1c592 Compare January 19, 2025 19:19
Copy link
Contributor

@azooz2003-bit azooz2003-bit left a comment

Choose a reason for hiding this comment

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

just change what i mentioned, else all good

Comment on lines +12 to +14
replacingOccurrences(of: "<[^>]+>", with: "", options: .regularExpression)
.replacingOccurrences(of: "&nbsp;", with: "")
.replacingOccurrences(of: "&amp;", with: "&")
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixes issue where announcements may show HTML entities (like &nbsp).

Comment on lines 39 to 48
guard announcement.course == nil,
let contextCode = announcement.contextCode else {
return announcement
}

announcement.course = courses.first(where: { course in
contextCode.contains(course.id)
})

return announcement
Copy link
Contributor

Choose a reason for hiding this comment

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

i like this

@rahulon12
Copy link
Member Author

@azooz2003-bit Can you take one more look? I've updated the PR

@azooz2003-bit
Copy link
Contributor

@azooz2003-bit Can you take one more look? I've updated the PR

sure, can you rebase w main again. i wanna make sure theres no more dup key errors.

@azooz2003-bit
Copy link
Contributor

@azooz2003-bit Can you take one more look? I've updated the PR

sure, can you rebase w main again. i wanna make sure theres no more dup key errors.

nvm, code looks good. u can merge. ill try after u merge

@rahulon12 rahulon12 merged commit e18422c into main Jan 20, 2025
@rahulon12
Copy link
Member Author

@azooz2003-bit try now

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.

2 participants