-
Notifications
You must be signed in to change notification settings - Fork 92
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
Feature/new footer #3701
Feature/new footer #3701
Conversation
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 component. I have some comments on the styles and style names as I am concerned about project pollution across future components. Please review.
return ( | ||
<MediaQuery minWidth={850}> | ||
{isDesktop => ( | ||
<div className="footerGfw"> |
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.
So whilst we are doing this lets update the code to be inline with our other components. Lets start using the same class structure etc. so rename this to c-footer
or similar.
$mobile-padding: rem(20px); | ||
$desktop-padding: rem(40px); | ||
|
||
.c-form-container { |
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.
Can we use better naming for these across all the modals?
} | ||
} | ||
|
||
.feedback-message { |
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 isn't scope to the class of the component. If another class is created with the same name it will be polluted.
}} | ||
> | ||
{ | ||
<div className="c-form-container"> |
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.
Better naming here.
$mobile-padding: rem(20px); | ||
$desktop-padding: rem(40px); | ||
|
||
.c-form-container { |
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.
And here.
text-transform: uppercase; | ||
color: #555; | ||
.c-contact-us { | ||
.c-contact-form { |
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.
Another thing we try to avoid is referencing component parent classes inside a css file. So in this case you are changing the components styles from the outside, but if there were multiple instances of this component rendered and you only wanted to change this instance you would have a problem. Can you try to assign a theming class here to adjust layout as desired.
width: 600px; | ||
padding: 1rem; | ||
margin: auto; | ||
.c-button { |
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.
same here.
Overview
This PR features a "new" footer for the entire site, intended to be the same as the old one but implemented in React, as well as 2 different modals ('Contact us' and 'Newsletter').