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

Add team sidebar display conditions #689

Merged
merged 2 commits into from
Oct 22, 2023
Merged

Add team sidebar display conditions #689

merged 2 commits into from
Oct 22, 2023

Conversation

sri240
Copy link
Contributor

@sri240 sri240 commented Oct 18, 2023

If the user is not logged in, the entire TEAM MANAGEMENT section of the sidebar should not appear
If the user is logged in and does not have a team, the Submissions and Scrimmaging sections should not appear
If the user is logged in and has a team, the entire TEAM MANAGEMENT section should appear

@acrantel acrantel changed the base branch from main to frontend2 October 18, 2023 21:17
Copy link
Member

@acrantel acrantel left a comment

Choose a reason for hiding this comment

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

Overall looks good! Left a comment about nullness / undefinedness.

It turns out that sidebar is the only component right now with tests, so we'll need to update those tests before merging this PR. I believe the change we'll need to make is to add a <CurrentUserContext.Provider> and <CurrentTeamContext.Provider> to wrap the <Sidebar> in the tests. You can find the test at components/sidebar/test/Sidebar.test.tsx

Comment on lines 101 to 102
if (!isNull(user) && user !== undefined) {
if (!isNull(team) && team !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

user and team should never be null (you can hover over them to see the types). This means we can just check !isUndefined(user) and !isUndefined(team) for these if statements

Copy link
Member

@acrantel acrantel left a comment

Choose a reason for hiding this comment

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

looks good to me! feel free to merge :)

@sri240 sri240 merged commit a314b1d into frontend2 Oct 22, 2023
3 checks passed
@sri240 sri240 deleted the sri/sidebar-status branch October 22, 2023 02:35
acrantel pushed a commit that referenced this pull request Feb 8, 2024
acrantel pushed a commit that referenced this pull request Feb 8, 2024
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