www
/
cs-2022-class-profile
16
0
Fork 0
Code Issues 11 Pull Requests 1 Projects Releases Wiki Activity

WIP: Creating component switcher #158

Closed
r2shuai wants to merge 2 commits from richardshuai/component-switcher into main
pull from: richardshuai/component-switcher
merge into: www:main
Conversation 2 Commits 2 Files Changed 4 +113 -14
r2shuai commented 2023-01-29 17:32:26 -05:00
Member
Copy Link

This is specifically for Issue #142.

Work in progress and please check if this is what we want for the component switcher. Thanks!

This is specifically for Issue #142. Work in progress and please check if this is what we want for the component switcher. Thanks!
r2shuai added 1 commit 2023-01-29 17:32:28 -05:00
continuous-integration/drone/push Build is failing Details
a4a012a7a6 Creating component switcher
r2shuai requested review from e26chiu 2023-01-29 17:34:43 -05:00
snedadah reviewed 2023-01-31 00:44:53 -05:00
snedadah left a comment
Owner
Copy Link

Nice work!
Yep, this is what we are looking for. I reccomend you add it to the top of the SamplePage so you can view and test it as you develop. (Copying some of those graphs as well)

https://git.csclub.uwaterloo.ca/www/cs-2022-class-profile/src/branch/main/pages/samplePage.tsx

The hard part is going to be having to style it. Let me know if you need any help or run into issues!

Nice work! Yep, this is what we are looking for. I reccomend you add it to the top of the SamplePage so you can view and test it as you develop. (Copying some of those graphs as well) https://git.csclub.uwaterloo.ca/www/cs-2022-class-profile/src/branch/main/pages/samplePage.tsx The hard part is going to be having to style it. Let me know if you need any help or run into issues!
👍 1
r2shuai added 1 commit 2023-02-01 23:03:21 -05:00
continuous-integration/drone/push Build is failing Details
408896098e component switcher and attempt to style
r2shuai requested review from snedadah 2023-02-01 23:03:56 -05:00
r2shuai commented 2023-02-01 23:07:41 -05:00
Author
Member
Copy Link

The current issue is: if two graphs are of different types (for example, pie and line), they might have different height.

The solutions are: 1. wrap the graph around a container that has a set height, but might disrupt its aspect ratio; 2. keep the way it is but the position might shift.

Also, the lint failed because I am currently not using an assigned state. We can change this later if everything else is complete.

Let me know if I need to change anything. Thanks for reviewing :)

The current issue is: if two graphs are of different types (for example, pie and line), they might have different height. The solutions are: 1. wrap the graph around a container that has a set height, but might disrupt its aspect ratio; 2. keep the way it is but the position might shift. Also, the lint failed because I am currently not using an assigned state. We can change this later if everything else is complete. Let me know if I need to change anything. Thanks for reviewing :)
e26chiu requested changes 2023-02-04 14:17:21 -05:00
e26chiu left a comment
Contributor
Copy Link

Hey @r2shuai ! Thanks for working on this issue, it's definitely more complex since we don't have the design for this component! In our case (or at least, in this term), the type of the graph shouldn't change (it should be the same for all terms! Have we considered trying to put the buttons above instead? How would that look?
Otherwise, here are the comments and questions I have for now! Good work so far!

Hey @r2shuai ! Thanks for working on this issue, it's definitely more complex since we don't have the design for this component! In our case (or at least, in this term), the type of the graph shouldn't change (it should be the same for all terms! Have we considered trying to put the buttons above instead? How would that look? Otherwise, here are the comments and questions I have for now! Good work so far!
👍 1
components/ComponentSwitcher.module.css
@ -0,0 +1,33 @@
.btn {
--button-color: #E55F98;
--border-color: #FEA0C8;
e26chiu commented 2023-02-04 14:07:05 -05:00
Contributor
Copy Link

Just a question, where did you choose these colours from? I think we should aim to reuse colours defined in the _app.css file as much as possible instead of definining new colours. It's also better if you define the variable names in that file so that it will be easier to change these values later on (since they are all mostly in that file).

Just a question, where did you choose these colours from? I think we should aim to reuse colours defined in the `_app.css` file as much as possible instead of definining new colours. It's also better if you define the variable names in that file so that it will be easier to change these values later on (since they are all mostly in that file).
r2shuai commented 2023-02-04 15:56:44 -05:00
Author
Member
Copy Link

These colors are taken directly from _app.css. I believe they are the primary and secondary accent colour. Let me know if I need to change them :)

These colors are taken directly from `_app.css`. I believe they are the primary and secondary accent colour. Let me know if I need to change them :)
components/ComponentSwitcher.module.css
@ -0,0 +23,4 @@
--button-color: #CA634D;
--border-color: #FFBC9F;
background-color: var(--button-color);
e26chiu commented 2023-02-04 14:12:39 -05:00
Contributor
Copy Link

There should be perhaps a cursor: pointer property to make the button seem clickable?

There should be perhaps a `cursor: pointer` property to make the button seem clickable?
👍 1
components/ComponentSwitcher.tsx
@ -0,0 +11,4 @@
buttonList,
graphList,
}: ComponentSwitcherProps) {
const [selectedButton, setSelectedButton] = useState("1A");
e26chiu commented 2023-02-04 14:09:08 -05:00
Contributor
Copy Link

Instead of hardcoding this value, we could perhaps use buttonList[0]?

Instead of hardcoding this value, we could perhaps use `buttonList[0]`?
r2shuai commented 2023-02-04 15:57:09 -05:00
Author
Member
Copy Link

Ok I will change that!

Ok I will change that!
r2shuai marked this conversation as resolved
r2shuai commented 2023-02-04 21:01:58 -05:00
Author
Member
Copy Link

Hey @r2shuai ! Thanks for working on this issue, it's definitely more complex since we don't have the design for this component! In our case (or at least, in this term), the type of the graph shouldn't change (it should be the same for all terms! Have we considered trying to put the buttons above instead? How would that look?
Otherwise, here are the comments and questions I have for now! Good work so far!

Hey @e26chiu I tried to move the buttons to the top, but I think it looks better in the bottom in general (especially when there're a lot of them). You can try to see which one looks better and let me know if it needs to be changed!

> Hey @r2shuai ! Thanks for working on this issue, it's definitely more complex since we don't have the design for this component! In our case (or at least, in this term), the type of the graph shouldn't change (it should be the same for all terms! Have we considered trying to put the buttons above instead? How would that look? > Otherwise, here are the comments and questions I have for now! Good work so far! Hey @e26chiu I tried to move the buttons to the top, but I think it looks better in the bottom in general (especially when there're a lot of them). You can try to see which one looks better and let me know if it needs to be changed!
r2shuai closed this pull request 2023-02-04 21:01:58 -05:00
r2shuai reopened this pull request 2023-02-04 21:06:43 -05:00
r2shuai closed this pull request 2023-02-04 21:10:24 -05:00
Some checks failed
continuous-integration/drone/push Build is failing
Required
Details

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
snedadah
e26chiu
Labels
Clear labels   
Bug

   
Component

   
Config

   
Good First Issue

   
Low-Priority

   
Page
No Label
Bug
Component
Config
Good First Issue
Low-Priority
Page
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#158
No description provided.
Delete Branch "richardshuai/component-switcher"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?