Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upDon't vertically clip creator header images in tip UI #484
Conversation
cg505
changed the title
|
Overall things are looking good, but can you add either a knob or a demonstration in to the storybook concept for siteBanner so we can see how it looks between images of varying heights? You can just make a toggle knob like shortBgImage/tallBgImage even :) |
|
@ryanml Actually all the banner images are the same aspect ratio, so the existing knobs are sufficient. The only thing to check is the different widths. |
I can attest to this, we store all of our images server side at 2700 x 528 That said, things could change in the future, might not hurt to create more knobs just for testing purposes. |
|
One thing that we need to keep is height of the image to be fixed, otherwise height will be too big and small screens will have problems. So what we need to achieve is, with height that we have defined, how to best fit it in there. |
|
@NejcZdovc Since the aspect ratio is so wide (900 by 176) you will have to have a really wide and really short screen for it to be a problem (like 2 or 3x as wide as tall). If we're worried about it, we could either cap the width of the image or vertically center it past a certain point. Not sure what the best approach would be in that case. |
|
The dialog box height is capped at 700px. We can add a max-height to the banner image so that everything should fit even if your screen is super wide. |
|
Pushed a change to cap the height of the banner image at 350px. This will avoid creating a scrollbar even on really wide screens. Looks like this: Do we want to handle this case some other way as well (like max-width or vertical centering)? |
|
Is there anyway we can handle these images in a way that doesn't clip, especially important if the image contains text @NejcZdovc @cg505 |
|
@dlipeles When the screen is really wide, we could somehow restrict the width of the banner image, but I'm not sure exactly how that will work. (What should be the background color? Should we restrict the width of the entire dialog?) |
|
I think setting a max-width on the entire banner might be a good idea, I will defer to @NejcZdovc |
|
Enforced a maximum width in the brave/brave-core#2566 so that the maximum height is no longer needed. |
| @@ -25,9 +25,16 @@ describe('WalletSummary tests', () => { | |||
|
|
|||
| describe('basic tests', () => { | |||
| it('matches the snapshot', () => { | |||
| const RealDate = Date | |||
| global.Date = class extends RealDate { | |||
| constructor () { | |||
This comment has been minimized.
This comment has been minimized.
This fixes vertical clipping of the header image when the browser is very wide. See brave/brave-browser#2015.
|
Graceful solve. Nice cleanup too! |
cg505 commentedMay 29, 2019
•
edited
Changes
See brave/brave-browser#2015. The banner image for the tip interface now scales height dynamically.
Also resolves #483 and #488.
Test plan
With header image
--rewards=staging=false.Without header image
Link / storybook path to visual changes
https://brave-ui-6cuwjq0t3.now.sh?path=/story/feature-components-rewards-concepts-desktop--site-banner (toggle "Show bg image" knob)
Integration
Does this contain changes to src/components or src/
Does this contain changes to src/features for brave-core?