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

#13 footer component #27

Merged
merged 5 commits into from
Mar 26, 2021
Merged

#13 footer component #27

merged 5 commits into from
Mar 26, 2021

Conversation

samgildea
Copy link
Contributor

lofi footer style and structure

Copy link
Contributor

@josephmannis josephmannis left a comment

Choose a reason for hiding this comment

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

Just a couple of little things! Looks good otherwise

import { StaticQuery, graphql } from "gatsby"

const FooterContainer = styled.div`
background-color: #848484;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you pull these colors out into variables pease?

}
`

const FooterHeader = styled.div`
Copy link
Contributor

Choose a reason for hiding this comment

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

If this type style is used in multiple places, can you pull it out into a typography.js file please?

}
`

const AddressSection = styled.div`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a general trend for project organization in Scout rn:

component-name/
   component.js
   component-styles.js
   <anything else related to the component>

When you get a lot of files this keeps stuff really easy to follow, would you mind organizing this way? Also, could you please keep the styles inside their own folder?

<AddressSection>
123 Lorem Ipsum Dolor Tempor Incididunt YZ, 12345 123-456-7890
</AddressSection>
{/* <EmailText>{data.prismicFooter.data.email}</EmailText> */}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you drop this comment please?

return (
<a href={social.social_link}>
<SocialPlaceholder>
{/* <img src={social.social_icon.url} /> */}
Copy link
Contributor

Choose a reason for hiding this comment

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

This too

<EmailSection>
<form>
<input
type="text"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add aria labels or names to these labels + forms? here is a nifty document

</EmailSection>
</FooterMainContent>

{/* <BottomLinks>
Copy link
Contributor

Choose a reason for hiding this comment

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

This as well

@samgildea samgildea merged commit fefedbe into master Mar 26, 2021
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