Update thank you page #786
Conversation
| } | ||
|
|
||
| .text { | ||
| margin-bottom: 18px; | ||
| color: $light-text; | ||
| font-size: 14px; |
There was a problem hiding this comment.
on #703 I added "font-size: $body-font-size-normal" to the html element, that sets the baseline across the whole app to 14px so there's no need to set it here.
For future reference, we have four font sizes defined on _fonts.scss: 12px (small), 14px (normal), 16px (large) and 18px (x-large).
| img { | ||
| display: inline-block; | ||
| } | ||
| margin-bottom: 0px; |
There was a problem hiding this comment.
small tweak but 0 doesn't take units so "margin-bottom: 0" would be valid CSS
|
|
||
| .header { | ||
| font-size: 1.5em; | ||
| margin-top: 0px; |
There was a problem hiding this comment.
small tweak but 0 doesn't take units so "margin-bottom: 0" would be valid CSS
| } | ||
|
|
||
| .header { | ||
| font-size: 1.5em; |
There was a problem hiding this comment.
I like that you're using ems and I think that's probably the way going forward when the app becomes responsive.
With our baseline at 14px 1.5em is rendered as 21px, the Sketch reference has headers set at 24px (1.7em approx.).
Minor conversation at this point but hey, every pixel counts. 😛
| .icon img { | ||
| display: inline-block; | ||
| .header, .text { | ||
| margin-bottom: 20px; |
There was a problem hiding this comment.
I'd set margins as ems as well, 14px is the baseline multiplier so 20px would translate to "margin-bottom: 1.4em"
| <div class="icon"><img src={{project.iconLargeUrl}} alt={{projectIconAltText}} data-test-selector="project icon" /></div> | ||
| <h3 class="header">Thank you!</h3> | ||
| <div class="text" data-test-selector="thank you message">From all the volunteers on the {{project.title}} team.</div> | ||
| <div class="project-link">{{#link-to "project" project}}Back to project{{/link-to}}</div> |
There was a problem hiding this comment.
This comment applies to all html elements above (and it's really just a suggestion). To follow the BEM approach described on #672 I'd make the following changes:
<div class="thankyou-container">
<div class="thankyou-container__message">
<div class="project-icon"></div> (more descriptive than just "icon" and could be used on other templates. If a class "icon" already exists you can extend project-icon with: @extend icon)
<h3>Thank you!</h3>
<p>From all the volunteers on the {{project.title}} team.</p>
{{#link-to "project" project class="project-link"}}Back to project{{/link-to}} (you can add a class directly to the view helper)
</div>
</div>
This would of course affect the css above and also the tests, I think it'd make it easier to style the component, most of the styling would be inherited.
|
I've got this. |
add thankyou-container page object add tests update src and alt of icon image try to update styling
joshsmith
force-pushed
the
update-thank-you-page
branch
from
6c4d80a to
3d6457b
Compare
What's in this PR?
alttext for project's icon on thank you pagesrcof project's icon on thank you pagethankyou-containercomponentthankyou-containercomponent page objectReferences
Progress on: #477