Skip to content
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

Project Design Handoff #48

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

LinaAdamsson
Copy link

No description provided.

@LinaAdamsson LinaAdamsson changed the title Project esign Handoff Project Design Handoff Apr 16, 2023
Copy link

@FionaKlacar FionaKlacar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done, Lina! This was a huge project and you got a lot of the major elements in! It looks like there is just some tidying up in some places with images and font weights etc, and some media queries to do. The overview section is definitely one of the most complicated - a tip is to put everything within borders as you work so that you can see what is happening and how things are moving on the screen. Also putting all the vector shapes within their own containers and making the containers position: relative and the shapes position: absolute. Good luck with it and well done!


export const App = () => {
return (
<div>
Find me in src/app.js!
<div className="landingPage">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious as to why you need a landing page classname if you’re not going to do any additional styling to it?

@@ -1,13 +1,22 @@
* {
box-sizing: border-box;
/* margin: 0; */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious as to why you commented out margin: 0 here and wonder if it affects your overspill on the x axis?

{/* <div className="navBar"> */}
<img className="logo" src={LogoSmallonLightBackground} alt="Raise Studio logo" />
<div className="menu">
<img className="hamburgerMenu" src={HamburgerMenu} alt="Menu icon - click to expand navigation menu" />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header looks great with the icon and the hamburger icon! Not much more to do to make it responsive - to create a tablet and desktop version, I don’t know if its best practice but I created a desktop version div within the component and just put display: none for the mobile version (and added the two more buttons needed to the mobile version for tablet).

.intro {
display: flex;
flex-direction: column;
height: 100vh;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the structure doesn't quite work here - I also put everything in one wrapper with display: flex and flex-direction: column, but without height: 100vh; maybe this would help keep this section separate from the surrounding sections? Because I think otherwise it wants to take up the whole viewport height.

Comment on lines +11 to +13
<h2>Barre Basic</h2>
<p>Our Beginner’s Class will give you a solid foundation on your...</p>
<button type="button" className="readMoreBtn">Read More</button>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smart to add this text for screen readers to work, instead of just using the text from the picture!

@@ -0,0 +1,21 @@
.quizContainer {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job making this quiz section responsive!

}


.vectorShape {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a hard time keeping these vector shapes in the right place! I got some advice to put them in their own parent container, and make the container position: relative and the shape position: absolute. (So the foreground shape is positioned somewhere precisely within its parent container).

Comment on lines +40 to +43
<span>I have read and understood the
<button type="button" className="registrationBtn"> Terms & Conditions</button> and
<button type="button" className="registrationBtn"> Privacy Policy</button>
</span>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you used span here instead of div to show it’s a section? The span element is an inline-level element used for styling or grouping inline content. So it typically groups code on one line rather than a block of code with multiple lines.

</label>
</div>
<span>Join Raise Studio</span>
<input type="submit" value="submit" className="joinRaiseStudioBtn" />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason you used input here instead of a button element, like below:

<button type="submit" className="joinbtn">Join</button> ?

@@ -0,0 +1,26 @@
.footerContainer {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job with the footer! All the elements are there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants