refactor: remove light/dark props from Card #41

Merged

klaipras merged 7 commits from remove-light-dark-props into ui_change 2026-10-17 22:36:14 +00:00

Conversation 7 Commits 7 Files changed 5 +71 -24

klaipras commented 2026-09-26 21:31:08 +00:00

Member

Copy link

Description

The props for card currently contain the "lightColor" and "darkColorProp" which is then used to determine the color of the component. The client calls in the different value depending on whether the card is selected or not. In the components itself, the value are also parsed into selected or not again. We could easily just used selected: boolean as prop instead of putting different prop; this still behave the same.

I understand that future feature might be able to make use of the colorprop but this is what works better at the moment

Related Issue

Closes #40

Type of change

• Bug fix
• New feature
• Refactor / code improvement
• Test coverage
• Documentation

Testing

• I added/updated tests.
• I tested my changes locally.
  • I tested my changes with a laptop-size screen.
  • I tested my changes with a phone-size screen.
  • I tested my changes with a tablet-size screen.
• I tested on the dev app (dev.cmustudy.com)

Screenshots/Recordings

Checklist

• My code follows the project's existing style.
• I have reviewed my changes.
• Only the necessary files and lines have been changed.
• This PR is ready for review.

## Description The props for card currently contain the "lightColor" and "darkColorProp" which is then used to determine the color of the component. The client calls in the different value depending on whether the card is selected or not. In the components itself, the value are also parsed into selected or not again. We could easily just used selected: boolean as prop instead of putting different prop; this still behave the same. I understand that future feature might be able to make use of the colorprop but this is what works better at the moment ## Related Issue Closes #40 ## Type of change - [ ] Bug fix - [ ] New feature - [x] Refactor / code improvement - [ ] Test coverage - [ ] Documentation ## Testing - [ ] I added/updated tests. - [x] I tested my changes locally. - [ ] I tested my changes with a laptop-size screen. - [ ] I tested my changes with a phone-size screen. - [ ] I tested my changes with a tablet-size screen. - [ ] I tested on the dev app (dev.cmustudy.com) ## Screenshots/Recordings ## Checklist - [x] My code follows the project's existing style. - [x] I have reviewed my changes. - [x] Only the necessary files and lines have been changed. - [x] This PR is ready for review.

klaipras added 1 commit 2026-09-26 21:31:08 +00:00

refactor: remove light/dark props from Card ... 0c51c42992

All current calls is just "selected" as bool,
can be readded if more color choice are needed

klaipras requested reviews from annadavi, mishag, cseluzhy 2026-09-26 21:31:27 +00:00

klaipras added 1 commit 2026-09-26 21:35:41 +00:00

refactor: remove the groupColor constants ... eea657e0fe

Unused now due to previous change

klaipras added a new dependency 2026-09-26 21:40:21 +00:00

#40 Redundant colors in Card component

annadavi force-pushed remove-light-dark-props from eea657e0fe to fb9880c18c 2026-09-28 03:03:01 +00:00 Compare

annadavi commented 2026-09-28 03:03:31 +00:00

Member

Copy link

I just added a PR template to the repo. In the future, it will automatically populate new PRs. Since this PR was made before the template existed, please just edit the description, copy the following template into it, and fill in the template:

## Description



## Related Issue


Closes #

## Type of change

- [ ] Bug fix
- [ ] New feature
- [ ] Refactor / code improvement
- [ ] Test coverage
- [ ] Documentation

## Testing

- [ ] I added/updated tests.
- [ ] I tested my changes locally.
    - [ ] I tested my changes with a laptop-size screen.
    - [ ] I tested my changes with a phone-size screen.
    - [ ] I tested my changes with a tablet-size screen.
- [ ] I tested on the dev app (dev.cmustudy.com)

## Screenshots/Recordings



## Checklist

- [ ] My code follows the project's existing style.
- [ ] I have reviewed my changes.
- [ ] Only the necessary files and lines have been changed.
- [ ] This PR is ready for review.

I just added a PR template to the repo. In the future, it will automatically populate new PRs. Since this PR was made before the template existed, please just edit the description, copy the following template into it, and fill in the template: <pre> ## Description <!-- Please provide a description of the issue, the changes you made to fix it, and why they work. It is expected that you understand why your changes work and if you do not understand why at least say as much so a maintainer knows how much to value the PR. No AI slop. --> ## Related Issue <!-- Link the issue this PR addresses, if applicable. --> Closes # ## Type of change - [ ] Bug fix - [ ] New feature - [ ] Refactor / code improvement - [ ] Test coverage - [ ] Documentation ## Testing - [ ] I added/updated tests. - [ ] I tested my changes locally. - [ ] I tested my changes with a laptop-size screen. - [ ] I tested my changes with a phone-size screen. - [ ] I tested my changes with a tablet-size screen. - [ ] I tested on the dev app (dev.cmustudy.com) ## Screenshots/Recordings <!-- If this includes a UI change, attach screenshots on both laptop and mobile. --> ## Checklist - [ ] My code follows the project's existing style. - [ ] I have reviewed my changes. - [ ] Only the necessary files and lines have been changed. - [ ] This PR is ready for review. </pre>

klaipras added 3 commits 2026-10-03 20:40:04 +00:00

refactor: remove light/dark props from Card ... 0c51c42992

All current calls is just "selected" as bool,
can be readded if more color choice are needed

refactor: remove the groupColor constants ... eea657e0fe

Unused now due to previous change

Merge branch 'remove-light-dark-props' of ssh://git.cmu.dev/ScottyLabs/study into remove-light-dark-props 5aa08fa481

annadavi commented 2026-10-04 02:28:09 +00:00

Member

Copy link

Can you link the issue in the description and add an explanation to the description of what you did and why, so we can understand the reason behind the changes/why you solved the issue this way?

Can you link the issue in the description and add an explanation to the description of what you did and why, so we can understand the reason behind the changes/why you solved the issue this way?

klaipras commented 2026-10-04 03:04:45 +00:00

Author

Member

Copy link

@annadavi I have explained more in the desc and like the issue.

@annadavi I have explained more in the desc and like the issue.

annadavi commented 2026-10-04 03:30:07 +00:00

Member

Copy link

Can you also add UI tests that verify the colors in light/dark mode and selected/not, just to build up the tests as we implement?

Can you also add UI tests that verify the colors in light/dark mode and selected/not, just to build up the tests as we implement?

klaipras commented 2026-10-04 03:37:32 +00:00

Author

Member

Copy link

I'm not sure how to implement a test for this kind of component

I'm not sure how to implement a test for this kind of component

annadavi commented 2026-10-04 04:04:16 +00:00

Member

Copy link

Take a look at the existing tests (and maybe tests added on other open PRs) and do some research on UI testing with Vitest, which is what we use for our tests. Basically, just add tests to confirm it looks as you expect (colors are correct for light/dark mode and selected vs not, so just 4 cases). This is mostly just to give you practice writing tests for your PRs, even if it seems a little silly on a small change like this!

Take a look at the existing tests (and maybe tests added on other open PRs) and do some research on UI testing with Vitest, which is what we use for our tests. Basically, just add tests to confirm it looks as you expect (colors are correct for light/dark mode and selected vs not, so just 4 cases). This is mostly just to give you practice writing tests for your PRs, even if it seems a little silly on a small change like this!

klaipras added 1 commit 2026-10-17 20:25:39 +00:00

feat: add test for card class 688889ac9a

klaipras added 1 commit 2026-10-17 20:32:47 +00:00

Merge branch 'ui_change' of ssh://git.cmu.dev/ScottyLabs/study into remove-light-dark-props 3b21ca7b0e

klaipras commented 2026-10-17 20:37:03 +00:00

Author

Member

Copy link

@annadavi I have added the test to check for cards props. It check for the class.

@annadavi I have added the test to check for cards props. It check for the class.

klaipras merged commit 0af978b24f into ui_change 2026-10-17 22:36:14 +00:00

klaipras deleted branch remove-light-dark-props 2026-10-17 22:36:15 +00:00

klaipras referenced this pull request from a commit 2026-10-17 22:36:16 +00:00

Merge pull request 'refactor: remove light/dark props from Card' (#41) from remove-light-dark-props into ui_change

Sign in to join this conversation.

Reviewers

No reviewers

cseluzhy

mishag

annadavi

Labels

Clear labels   

first-task

  

needs/docs

Must have documentation written

  

needs/research

Discuss the ideal implementation with the tech lead

  

needs/upstream

Needs a contribution to an upstream project

  

type/bug

Something not working as intended

  

type/external

Lives outside of code

  

type/feature

New functionality

  

type/refactor

Improving existing code

  

urgency

high

This is currently causing issues

  

urgency

immediate

This is making a project or feature unusable

  

urgency

low

This can happen whenever

  

urgency

medium

This is expected to cause issues soon

No labels

first-task

needs/docs

needs/research

needs/upstream

type/bug

type/external

type/feature

type/refactor

urgency

high

urgency

immediate

urgency

low

urgency

medium

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Depends on

#40 Redundant colors in Card component

ScottyLabs/study

Reference

ScottyLabs/study!41

No description provided.