Donation after-actions makeover CSS #304
Conversation
technicalex
left a comment
technicalex
left a comment
There was a problem hiding this comment.
Looks good! Just a reminder about a change you already made elsewhere :)
codygordon
left a comment
codygordon
left a comment
There was a problem hiding this comment.
I can't see the Figma, if you're able to share it. Just have some subjective design feedback: it's hard to tell the survey input is an input as it's white on white, and the Yes/No radio at the bottom looks a bit crowded and some margin-top on it would help!
|
The background was supposed to be light blue, thanks for catching that! I made a mistake trying to fix something with the templateset, should be better now. Added a little extra spacing for the radio buttons as well. |
|
[On pause while I address feedback from Bri] |
|
I had to make a few additional changes; requesting feedback again. Thanks! |
technicalex
left a comment
technicalex
left a comment
There was a problem hiding this comment.
Just a few questions. Thanks!
| @@ -0,0 +1,4 @@ | |||
| <?xml version="1.0" encoding="UTF-8" standalone="no"?> | |||
There was a problem hiding this comment.
Similar question to the one I left in the ak-template-set pull request: is this file used anywhere? If so, I can't seem to find it.
There was a problem hiding this comment.
ak-template-set instead includes the .html version, but it still seemed useful to have it in Giraffe too. If you think it's best to just leave in ak-template-set I can delete here.
|
|
||
| html { | ||
| width: 100vw; | ||
| scroll-padding-top: 57px; |
There was a problem hiding this comment.
I wasn't familiar with this style, so I looked it up. I think it only applies when we are using scroll snap, which I can't find either in existing styling or in these changes. Maybe I'm missing something; can you please explain? Thank you!
There was a problem hiding this comment.
On mobile, there's a button that lets you jump to the form, but because the header floats above everything else on the page, the form gets cut off. Adding the padding here makes the anchor tag jump to the right place to display the entire form.
There was a problem hiding this comment.
FYI @sjwmoveon, new scroll-padding-top values should be used for compatibility with the new header if this pull request is merged. I've copied the correct values below. (I suppose only the first one applies, if the button to jump will only ever appear on mobile.) Thank you!
html {
scroll-padding-top: 145px;
}
@media (min-width: 768px) {
html {
scroll-padding-top: 149px;
}
}
@media (min-width: 1025px) {
html {
scroll-padding-top: 102px;
}
}
| $moriginal-blue-medium: #296ecb; | ||
| $c-facebook: #3B5998; | ||
| $c-twitter: #1DA8E2; | ||
| $c-twitter: #1DA8E2; |
There was a problem hiding this comment.
This color doesn't appear to be used. Furthermore, it looks like it's a different color from the color specified on Figma (#5AA5EB) and also (confusingly!) different from both the button and hover color I was asked to use for the Twitter button on my page (#1DA1F2 and #3FB6FF). If it's not being used, I would recommend removing it, unless we expect to use it in the future. But maybe more importantly, should we ask for clarification on the correct Twitter color?
There was a problem hiding this comment.
I didn't actually add this, just a newline at the end of the file. But yes, that's very confusing and we should ask for clarification. (Maybe it's used on existing main-giraffe template pages?)
|
SonarCloud Quality Gate failed.
|
Just about everything is either tied to a particular object ID that's only on the new page or otherwise doesn't apply to existing pages.