-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Website] - Changes to the Landing Page & Team Page #289
Conversation
adding NBC Video to home page as well as sponser
updated the 4.0 logo and changed the section heading font sizes
updated section headings to one singular component as well as fixing the team page to actually display and show Yong Lin's photo
public/images/4.0.svg
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is nitpicky but can we rename this file so that it doesn't include a period in the filename itself? 4point.svg or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point -- 4.0 oftentimes uses 4pt0, so we might go with that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect, nice one
public/images/Schmidt Futures.svg
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, if we are using hyphens between words (like with andy-circle.png) we should probably be consistent throughout the folder. So this would be coming schmidt-futures.svg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just renamed it with the rest of the logos!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
package.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you both need to make these changes to get the environment running? It's probably not a big deal but can you provide some context to explain why each of the versions are changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually don't know what happened but currently we both have the node-sass: 8.0 version on our local folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm, ok sounds good. let's go with these versions then.
src/landingPage.scss
Outdated
height: 100%; | ||
z-index: 3; | ||
top: 0; | ||
left: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: indent to match line 239
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks way better, thanks
src/landingPage.scss
Outdated
width: 100%; | ||
height: 100%; | ||
position: relative; | ||
padding-bottom: 56.25%; /* 16:9 aspect ratio */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you mind giving me a bit more info on the padding-bottom
property here? I'm wondering how the padding itself impacts the layout and how you arrived at 56.25%
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're using SASS, so this value can be extracted into a variable and used across the file instead of duplicating the value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can explain the padding-bottom value. When I switched the video it would get these white borders on the sides, I'm assuming because of the aspect ratio not matching the container, so I ask ChatGPT and it give my this bit of code to fix the issue and that value is based on the aspect ratio of the video. 9/16
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, learned something new every day. Apparently, iframes
don't work like images in terms of scaling. Maybe it's just me but I think including this link: http://alistapart.com/article/creating-intrinsic-ratios-for-video/ would help as a comment (instead of 16:9 aspect ratio) so that other people who see this know where the padding-bottom
is coming from.
And Mani's point is a good one. you can define a variable in the scss file like: $video-aspect-ratio: 56.25%;
(underneath the imports in the file) and then use it like padding-bottom: $video-aspect-ratio
(in .right-on-video-container
). See this for reference: https://sass-lang.com/documentation/variables and shoot me a message on slack if you need a hand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha I did add the variable and placed the link in the file since it does a better job of explaining how that works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
totally, I did some testing and got the exact same result but didn't know about the padding-bottom trick. Good to know! I think adding the url will help clarify this if it isn't common knowledge. nice job
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job on the first PR, this is amazing, especially the new components you've created. I left some minor notes and also:
- Checkout markdown on Github for learning better about rich text editing for PR notes and generally anywhere in Github
- It's always nice to include a screenshot of the work when your changes is adding a new UI component or changing the existing behavior. You can even attach a video to your PRs
src/teamPage.scss
Outdated
@@ -33,6 +33,9 @@ | |||
letter-spacing: 0em; | |||
text-align: center; | |||
color: white; | |||
justify-items: center; | |||
display: grid; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra line, you can also remove the other extra line that was here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cindyqquach @jasonmosley21 do you mind double checking that you've resolved Mani's formatting comments? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I think we got all of them!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
src/teamPage.scss
Outdated
@@ -41,6 +44,8 @@ | |||
border: 3.5px solid #E29B5D; | |||
opacity: 1; | |||
justify-content: center; | |||
align-items: center; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra line, usually there shouldn't be an extra line before }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
src/landingPage.scss
Outdated
width: 100%; | ||
height: 100%; | ||
position: relative; | ||
padding-bottom: 56.25%; /* 16:9 aspect ratio */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're using SASS, so this value can be extracted into a variable and used across the file instead of duplicating the value
src/landingPage.scss
Outdated
@@ -103,6 +128,12 @@ | |||
margin-left: 15px; | |||
} | |||
|
|||
.supporters{ | |||
display: flex; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra line here and generally all over this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the file and tried to remove any extra lines!
</Grid> | ||
<RightOnVideo/> | ||
<Grid item sm={12} style={{ zIndex:2 }}> | ||
<SectionHeading title="Why RightOn"style={{ zIndex:2 }}/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a question mark in the title?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was supposed to me an exclamation mark which has been fixed!
src/components/atoms/Logos.jsx
Outdated
<Grid item style={{ height: '130px' }}> | ||
<div style={{height: '130px'}} className='supporters'> | ||
<img | ||
src={featureImage} | ||
alt="right-on-education-product-features" | ||
style={{ marginLeft: "60px", marginRight: "60px" }} | ||
justifyContent="center" alignItems="center" | ||
|
||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Indent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
{supporters.map((logos, key) => { | ||
return ( | ||
<Grid item> | ||
<Logos key={key} logos={logos} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something here, but what's this component for? Is this for showing the logo for each supporter? Can one support have multiple logos? Or is it just one logo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each supporter has just one logo. I used this component to handle organizing the logos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I think we can probably simplify this. I think in terms of layout we have a title ("With Support From") and then a grid containing the logos of the sponsors. We are using both Box
and Grid
mui
components as containers.
The mui
Grid
component is nice because it handles reorienting multiple components in a grid layout (particularly across different sized devices). So I think we only want to use it when we have multiple items in our container. We also can get all the functionality we need out of it with just one Grid container
and a series of Grid items
. In other cases we can just use Box
as a generic mui container.
If we follow this logic, we can actually just hoist the img
from Logos
and put it into this component, removing the other grid items in that component. So I think we would end up with this:
<Logos key={key} logos={logos} /> | |
function Supporters(props) { | |
const { supporters } = props | |
return ( | |
<Box component="section"> | |
<Box justifyContent="center" alignItems="center" > | |
<SectionHeading title="With support from" /> | |
</Box> | |
<Grid container justifyContent="center" alignItems="center" spacing={5} > | |
{supporters.map((logo) => ( | |
<Grid item> | |
<img | |
src={logo.featureImage} | |
alt="right-on-education-product-features" | |
style={{ | |
height: '130px', | |
width: '350px', | |
marginLeft: "60px", | |
marginRight: "60px" | |
}} | |
/> | |
</Grid> | |
))} | |
</Grid> | |
</Box> | |
); | |
} | |
(note that the above suggestion replaces everything from lines 11 - 40 in the original Supporters.jsx and allows Logos.jsx to be deleted.
@cindyqquach maybe give this a shot and see what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I update the code and it works fine, I just had to adjust the height and width a little. Should be good now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @jasonmosley21, got confused on who was working on what here. Thanks for addressing this.
<Grid | ||
item | ||
container | ||
spacing={5} | ||
justify="center" | ||
className="why-cards-section" | ||
> | ||
{supporters.map((logos, key) => { | ||
return ( | ||
<Grid item> | ||
<Logos key={key} logos={logos} /> | ||
</Grid> | ||
); | ||
})} | ||
</Grid> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Indent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
src/components/atoms/SingleWhy.jsx
Outdated
src={topBackground} | ||
className='why-section-top-background' | ||
// width="100%" | ||
alt="math-background" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Indent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
updated the NSF logo, and fix issues with the video component responsiveness. background texture is a work in progress
added width to the logos to center them and also adding some margin to the grid to increase spacing from the footer
for the table view I made the line height smaller. and added spacing between the text and underline.
@cindyqquach @jasonmosley21 Appreciate that you both added some descriptions to the PR - that helps a lot in understanding the changes. Would you also be able to add some screenshots to the description per Mani's comment above? Feel like it makes it a ton easier to understand what the PR is about. |
updates the frame around the logos to have less space, updated the right-on-video-container comments to help other understand the values, and updating indentation and removing spaces
updated the code to be a little more simplified. with that i had to adjust the logos again to be more aligned.
the section heading was behind the background
On the RightOnVideo.jsx we have swapped out the current video with the NBC featured video.


We've also moved the WhyRightOn down within the LandingPage.jsx.
Before:
After:
Then we've added a new Supporters.jsx into the LandingPage.jsx with funder logos.
For all the section titles on the Landing Page we reduced the font size and applied a new classname: landing-page-titles
For the Team page, we added new member profiles by adding photos and 3 new blocks in teamData.js
We also added an advisor tab to the carousel by editing the teamData.js and the rotating carousel function in ProfileAdvisors.jsx
We also add more media queries to the App.scss and landingPage.scss to fix the responsive of the header, header logo and footer logo size on the tablet screen.

Before:
After:
