Add About Component #48

Merged
j285he merged 7 commits from j285he-about into main 5 months ago
Collaborator
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 5 months ago
4915466f34 Add about
j285he requested review from a258wang 5 months ago
j285he requested review from snedadah 5 months ago
j285he requested review from e26chiu 5 months ago
j285he requested review from b72zhou 5 months ago
a258wang reviewed 5 months ago
.aboutWrapper {
position: relative;
flex-direction: column;
Owner

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
}
.about {
width: 90%;
Owner

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
width: 90%;
display: flex;
flex-direction: row;
background-color: var(--primary-background);
Owner

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
}
.about p {
color: var(--secondary-text)
Owner

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?
Poster
Collaborator

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 5 months ago
d3f1f08ff7 Code review fixes
j285he added 1 commit 5 months ago
829d809a01 Merge main
j285he added 1 commit 5 months ago
959e094a07 Merge branch 'main' into j285he-about
Owner

@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
a258wang approved these changes 5 months ago
a258wang left a comment
Owner

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
.about aside {
flex: 1;
margin-right: 40px;
Owner

NIT: rem

NIT: rem
j285he marked this conversation as resolved
j285he added 2 commits 5 months ago
j285he merged commit fc5600cb20 into main 5 months ago
j285he deleted branch j285he-about 5 months ago
j285he referenced this issue from a commit 5 months ago

Reviewers

snedadah was requested for review 5 months ago
e26chiu was requested for review 5 months ago
b72zhou was requested for review 5 months ago
a258wang approved these changes 5 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as fc5600cb20.
Sign in to join this conversation.
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/cs-2022-class-profile#48
Loading…
There is no content yet.