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

Kl628/inactive-student-display #483

Closed
wants to merge 2 commits into from

Conversation

kevin-lin12
Copy link
Contributor

@kevin-lin12 kevin-lin12 commented Nov 26, 2023

Summary

This pull request fixes the display for Inactive students and changes the button to a toggle

  • Only show inactive students when toggled
  • Changed button to toggle

Trello Link:
https://trello.com/c/MuY5PHBE/504-active-students-toggle

Test Plan

Style Changes
Before
Screenshot 2023-12-03 at 8 40 45 PM

After
Screenshot 2023-12-03 at 8 41 09 PM
Screenshot 2023-12-03 at 8 41 13 PM

Before Clicking Button
Screenshot 2023-12-03 at 8 38 10 PM

After Clicking Button
Screenshot 2023-12-03 at 8 38 17 PM

Notes

(12/3) Committed Button Style Change

Breaking Changes

@kevin-lin12 kevin-lin12 requested a review from a team as a code owner November 26, 2023 07:35
@dti-github-bot
Copy link
Member

dti-github-bot commented Nov 26, 2023

[diff-counting] Significant lines: 22.

@kevin-lin12 kevin-lin12 changed the title Fixed Inactive Student Display Kl628/inactive-student-display Nov 26, 2023
Copy link
Contributor

@CollinWoo CollinWoo left a comment

Choose a reason for hiding this comment

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

Great work Kevin! The toggle works perfectly when I tested it on my end. There's really only one very minor stylistic thing I would change.

Show inactive students
</label>
<button
type="button"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can omit type="button" here since the button tag already indicates that this element is a button. This shouldn't affect any of the functionality.

@@ -45,4 +45,4 @@
"url": "https://github.com/cornell-dti/carriage-web/issues"
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume that these changes to package.json and package-lock.json were unintentional. To make git ignore local changes to these files in the future, you can run git update-index --skip-worktree package-lock.json package.json.

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.

5 participants