-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Checkout: Thank You: Update support section in footer with new copy #3755
Conversation
The mockup has a lot more vertical padding. Should we maybe have a smaller gravatar for mobile? cc @mikeshelton1503 |
9cede83
to
57df7eb
Compare
I added more padding and also made sure the gravatar's size is consistent with the size of gridicons. |
57df7eb
to
8f2a871
Compare
} | ||
|
||
.checkout-thank-you__footer-content { | ||
margin: 16px auto; | ||
margin-left: auto; | ||
margin-right: auto; |
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.
Why not write this margin: 0 auto;
and save a line?
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.
Because in that case you're also resetting top and bottom margins. Moreover when it's explicit like this you don't have to think about it (and my old brain likes that), and it will be compiled as a single property/line by SASS anyway.
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.
it will be compiled as a single property/line by SASS anyway.
That's cool. I didn't know this.
} | ||
); | ||
return ( | ||
<a className="button" href={ url } target={ target }> |
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.
What do you think about using <Button />
for this? I assume you intended to use it, because it is imported at the top, but never used.
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 started with <Button />
but switched to a simple anchor because I assumed that this component couldn't allow me to manage the target
attribute. Looks like I was plain wrong.
I had a couple minor notes which I addressed in a commit that I pushed onto this branch. Feel free to squash it in with your commits, or delete it if I missed something. Otherwise, the code LGTM. 👍 |
@@ -51,13 +69,27 @@ const CheckoutThankYouFooter = React.createClass( { | |||
return ( | |||
<Card className="checkout-thank-you__footer"> | |||
<div className="checkout-thank-you__footer-content"> | |||
{ this.renderGravatar() } |
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 has become a nice component. Can we turn it into an app-component we can reuse outside of thank you pages? Thinking something like SupportFromHappiness
that we can render in devdocs/app-components
, on the /plans
page, etc.
(It would also help clean up the thank-you stylesheet.)
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.
Sure thing.
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.
Thanks :)
4a792e0
to
4025f7b
Compare
Thank you everyone for your feedback! I've made the suggested changes and this pull request is now ready for a last look. |
I still think we need placeholders, since the URL changes depending on the product we're loading. Having the HE not change is nice, but the main issue is around the URL. Especially in cases such as the one Drew described, in which the page can take a while to fully load, there is a risk the user will click that button and be taken to the wrong support site. |
I agree with @fabianapsimoes, let's go with placeholders. It eliminates the risk of a user going to the wrong place. |
b860f4b
to
6276965
Compare
I've rebased, updated existing placeholders to better match the expected layout of the |
👍 |
Simpler is better.
This makes sure the same gravatar is always displayed even if the component is rerendered.
6276965
to
15b6e36
Compare
Product 👍 |
Checkout: Thank You: Update support section in footer with new copy
This pull request addresses part of #2484 by updating the footer of the
Thank You
page with a new copy for the support section:Testing instructions
git checkout update/thank-you-footer
and start your serverThank You
page displays as expected for large viewportsThank You
page displays as expected for small viewportsAdditional notes
I wasn't able to align vertically the gravatar, especially since there is a difference of placement for the buttons depending on the size of the viewport.
Reviews