Moving interactive banner to the top #1007
Conversation
Verified that the package manager install steps work on the newest version of amazon linux
|
Any reason for the CSS changes? With no changes (mobile)
With changes (mobile)
With no changes (1440x698)
With changes (1440x698)
Note the differences on the background image. Do these changes work as expected on in IE11 and other browsers with partial support for Flexbox? Edit: didn't read this line on pr description
|
|
@lpinca Thanks for making the screenshots! (I totally should've done that) The main reason for decreasing the size of the images is to make the overall banner a bit smaller since it's going at the top. It's quite tall at the bottom of the page today and putting that at the top would overwhelm the rest of the content. The background change is just a result of the height of the container becoming smaller. As for flexbox support, I didn't add any flexbox styles, only refactored the existing ones. If it works today, it should work in this PR. You're right on about browser support, though. It looks like with partial support in IE10 and 11, the two images in the banner should still appear correctly. Any IE before that would likely only show the images butted up against each other. That's just going by documentation, though, so if anyone has those browsers handy... Thoughts? |
|
I would just move the banner at the top with no CSS changes. I agree that it may look a bit too big but I like to see the images in their entirety. On top of this it is also consistent with the "old" banner and it is granted to work in most browsers including IE. This is only my personal opinion though. Let's wait for others to chime in. |
|
@lpinca Sounds good. Thanks for the feedback! Just in reference to the CSS changes, the original issue also expressed the intent to make the banner shorter, which gave me the idea initially. |
True that, but the page is almost empty, I don't think it will have a big impact. |
|
thanks all for the work on this! I agree with applying to all locales @ryanmurakami and one last thing (sheepish look) I noticed that the background image is Amsterdam - attached here is the Austin background image - if it's too much hassle to change no biggie. |
|
@gtewallace Glad you caught that! I updated it with the new image after lowering the size and making it darker to match the levels of the Amsterdam pic. Here's what it looks like now: @lpinca I went ahead and removed the image constraints as well. Let me know if there's anything else! |
| @@ -0,0 +1,17 @@ | |||
| #interactive-wrapper | |||
| flex none | |||
| background-image url("/static/images/interactive/background.jpg") | |||
|
@ryanmurakami one last thing, compress the image. Edit: it's a jpg :) |
|
@lpinca I compressed the image with photoshop. Good enough? |
|
I guess so, what's the size? |
|
@lpinca 73kb now, compared to 382kb original. I tried to match the Amsterdam image, which was 70kb. |
|
Perfect! |
|
@lpinca :-D And I moved the background style to the |
|
Hmm compression changed the image a bit too much, look at the light, the new one is very dark! |
|
@lpinca I actually did that on purpose. The current Amsterdam image levels are darker so the logo/dates pop on top. The original was too light and made it difficult to see the info above it. I tried to match the levels to the Amsterdam image. I could lighten it a little, but I think it would conflict too much with the other images. |
|
Oh ok, got it. |
|
@ryanmurakami, thanks! |
|
@lpinca No problem! Thanks for the feedback! :-) |
Fixing #1004
Moving the interactive banner to the top of the page, refactoring styles, and bringing down the size of the images to look a little nicer.
*note: I wasn't able to test in IE or Edge, but looks good in Chrome/Firefox/Safari