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

Add About Component #48

Merged
j285he merged 7 commits from j285he-about into main 2022-09-12 10:20:11 -04:00
Conversation 1 Commits 7 Files Changed 4 +130 -1
j285he commented 2022-09-01 01:31:02 -04:00
Member
Copy Link

Closes #40.

Will look correct when !46 is merged in.

https://j285he-about-csc-class-profile-staging-snedadah.k8s.csclub.cloud/

Closes #40. Will look correct when !46 is merged in. https://j285he-about-csc-class-profile-staging-snedadah.k8s.csclub.cloud/
j285he added 1 commit 2022-09-01 01:31:03 -04:00
continuous-integration/drone/push Build is passing Details
4915466f34 Add about
j285he requested review from a258wang 2022-09-01 01:31:08 -04:00
j285he requested review from snedadah 2022-09-01 01:31:11 -04:00
j285he requested review from e26chiu 2022-09-01 01:31:17 -04:00
j285he requested review from b72zhou 2022-09-01 01:31:21 -04:00
a258wang reviewed 2022-09-03 01:21:58 -04:00
components/About.module.css
@ -0,0 +1,55 @@
.aboutWrapper {
position: relative;
flex-direction: column;
a258wang commented 2022-09-03 01:12:49 -04:00
Owner
Copy Link

Is flex-direction: column necessary? Since the wrapper only has 1 non-absolutely-positioned child

Is `flex-direction: column` necessary? Since the wrapper only has 1 non-absolutely-positioned child
j285he marked this conversation as resolved
components/About.module.css
@ -0,0 +6,4 @@
}
.about {
width: 90%;
a258wang commented 2022-09-03 01:14:05 -04:00
Owner
Copy Link

Is there any reason why we aren't doing width: 100% (or maybe it's not even necessary to specify the width) and then just letting padding/margin handle the spacing?

Is there any reason why we aren't doing `width: 100%` (or maybe it's not even necessary to specify the width) and then just letting padding/margin handle the spacing?
j285he marked this conversation as resolved
components/About.module.css Outdated
@ -0,0 +9,4 @@
width: 90%;
display: flex;
flex-direction: row;
background-color: var(--primary-background);
a258wang commented 2022-09-03 01:15:21 -04:00
Owner
Copy Link

Is the background-color necessary here? I would have thought that it'd just be transparent by default, which would be fine since the page background colour is what we want.

Is the `background-color` necessary here? I would have thought that it'd just be transparent by default, which would be fine since the page background colour is what we want.
j285he marked this conversation as resolved
components/About.module.css Outdated
@ -0,0 +31,4 @@
}
.about p {
color: var(--secondary-text)
a258wang commented 2022-09-03 01:21:41 -04:00
Owner
Copy Link

I know this is a Figma thing and not an us thing, but why is the text here not just the usual white? 🥴 (Is it because the angle brackets are white? Why are the angle brackets white and not one of the usual accent colours?)

It's your choice, do whatever you think looks best. (Personally, I wonder what pink aside text + white main text would look like?)

Though I do want to ask if we really need a --secondary-text variable, or if using --primary-accent-lighter is sufficient?

I know this is a Figma thing and not an us thing, but *why* is the text here not just the usual white? 🥴 (Is it because the angle brackets are white? Why are the angle brackets white and not one of the usual accent colours?) It's your choice, do whatever you think looks best. (Personally, I wonder what pink aside text + white main text would look like?) Though I do want to ask if we really need a --secondary-text variable, or if using --primary-accent-lighter is sufficient?
j285he commented 2022-09-07 23:15:21 -04:00
Author
Member
Copy Link

I will remove --secondary-text (from light theme as well).

Also agree with the colours, how about this as a colour scheme?

I will remove --secondary-text (from light theme as well). Also agree with the colours, how about this as a colour scheme?
j285he added 2 commits 2022-09-07 23:15:33 -04:00
continuous-integration/drone/push Build is passing Details
d3f1f08ff7 Code review fixes
j285he added 1 commit 2022-09-07 23:16:18 -04:00
continuous-integration/drone/push Build is passing Details
829d809a01 Merge main
j285he added 1 commit 2022-09-09 17:29:57 -04:00
continuous-integration/drone/push Build is passing Details
959e094a07 Merge branch 'main' into j285he-about
a258wang commented 2022-09-11 02:40:46 -04:00
Owner
Copy Link

@j285he The normal coloured text is definitely an improvement. I personally think the "About the Programs" header should be at least as dark as the "Computer Science"/"Computing and Financial Management"/"CS/BBA" headers - probably primary-accent for one and secondary-accent for the other? Or maybe x-accent for "About the Programs" and x-accent-light for the other. Here are some examples:

image

  • About the programs = primary-accent
  • Getting an overview... = primary-accent-lighter
  • CS/CFM/CSBBA = secondary-accent
  • Angle brackets = secondary-accent-light

image

  • About the programs = secondary-accent
  • Getting an overview... = primary-accent-lighter
  • CS/CFM/CSBBA = primary-accent
  • Angle brackets = primary-accent-light
@j285he The normal coloured text is definitely an improvement. I personally think the "About the Programs" header should be at least as dark as the "Computer Science"/"Computing and Financial Management"/"CS/BBA" headers - probably primary-accent for one and secondary-accent for the other? Or maybe x-accent for "About the Programs" and x-accent-light for the other. Here are some examples: ![image](/attachments/50d6bee6-1f10-4a7b-a65c-8e167b8be8c2) - About the programs = primary-accent - Getting an overview... = primary-accent-lighter - CS/CFM/CSBBA = secondary-accent - Angle brackets = secondary-accent-light ----- ![image](/attachments/2dcea71b-5526-40d7-bd8d-1cdfadd7c993) - About the programs = secondary-accent - Getting an overview... = primary-accent-lighter - CS/CFM/CSBBA = primary-accent - Angle brackets = primary-accent-light
image.png
172 KiB
image.png
171 KiB
image.png
171 KiB
image.png
👍 1
a258wang approved these changes 2022-09-11 02:41:49 -04:00
a258wang left a comment
Owner
Copy Link

See my comment about maybe changing the colours up a bit more, but ultimately I'll leave it up to you - it's easy to change colours in the future if we want

See my comment about maybe changing the colours up a bit more, but ultimately I'll leave it up to you - it's easy to change colours in the future if we want
components/About.module.css
@ -0,0 +11,4 @@
.about aside {
flex: 1;
margin-right: 40px;
a258wang commented 2022-09-11 02:30:27 -04:00
Owner
Copy Link

NIT: rem

NIT: rem
j285he marked this conversation as resolved
j285he added 2 commits 2022-09-12 10:13:47 -04:00
continuous-integration/drone/push Build is passing Details
5657414036 Change colors
j285he merged commit fc5600cb20 into main 2022-09-12 10:20:11 -04:00
j285he deleted branch j285he-about 2022-09-12 10:20:11 -04:00
j285he referenced this issue from a commit 2022-09-12 10:20:11 -04:00
Add About Component (#48)
Sign in to join this conversation.
Reviewers
No reviewers
snedadah
e26chiu
b72zhou
a258wang
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
2 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#48
No description provided.
Delete Branch "j285he-about"

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?