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

Wrapped Frontend Render Safety Checks #903

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

rgu0114
Copy link
Contributor

@rgu0114 rgu0114 commented Nov 21, 2024

Summary

Logic fixes and safety checks to make sure data exists. Also refactored code and corrected number of slides. Made the fav [blank] slide data loaded in and affecting slides displayed as one unit.

Test Plan

Tested on prod database

@rgu0114 rgu0114 requested a review from a team as a code owner November 21, 2024 23:34
@dti-github-bot
Copy link
Member

dti-github-bot commented Nov 21, 2024

[diff-counting] Significant lines: 79.

Copy link
Contributor

@NIDHI2023 NIDHI2023 left a comment

Choose a reason for hiding this comment

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

just these few changes and should be good!

src/components/includes/Wrapped.tsx Outdated Show resolved Hide resolved
src/components/includes/Wrapped.tsx Show resolved Hide resolved
@rgu0114 rgu0114 mentioned this pull request Nov 22, 2024
2 tasks
@NIDHI2023
Copy link
Contributor

NIDHI2023 commented Nov 22, 2024

Not related to this PR but as future side note:
const StudentsHelpedBanner = () => ( <div> <Asterik/> YOU HAD THE MOST VISITS IN {month} <Asterik/> YOU HAD THE MOST VISITS IN {month} <Asterik/> </div> );
month is for the most common month you as a user visited office hours while this needs the month in which the most students came, so we'd need to make a separate variable/calculation for what's on this banner!

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.

3 participants