Skip to content
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

update brave-ui for tip site banner image fix #2566

Merged
merged 2 commits into from
Jul 1, 2019

Conversation

cg505
Copy link
Contributor

@cg505 cg505 commented May 31, 2019

See brave/brave-ui#484.
Changes from brave-ui: brave/brave-ui@1fd91fa...0220a24

Resolves brave/brave-browser#2015

Submitter Checklist:

Test Plan:

See test plan in brave/brave-ui#484.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@cg505 cg505 requested review from ryanml and NejcZdovc May 31, 2019 15:46
@cg505 cg505 force-pushed the site-banner-height-fix branch from 8043a41 to 389cdd5 Compare June 3, 2019 18:40
@cg505 cg505 force-pushed the site-banner-height-fix branch from 389cdd5 to f0ea60f Compare June 3, 2019 19:27
@ryanml ryanml requested a review from petemill June 4, 2019 16:28
@cg505 cg505 added this to the 0.68.x - Nightly milestone Jun 4, 2019
@cg505 cg505 force-pushed the site-banner-height-fix branch from f0ea60f to 24d9464 Compare June 18, 2019 21:23
@cg505 cg505 requested a review from ryanml June 18, 2019 23:24
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested works great. Apart from the changes requested, I'm worried about a flash-of-change-of-height when the image loads after the dialog displays and then subsequently pushes the dialog taller. However, I would be happy to get this in to a build and see how serious the issue is in the real world. We could then either revert or fix in a number of different ways.

browser/brave_rewards/tip_dialog.cc Outdated Show resolved Hide resolved
@cg505
Copy link
Contributor Author

cg505 commented Jun 19, 2019

I'm not sure what causes the initial flash at the wrong size. If it's based on some provided height estimate, we can adjust that.

@cg505 cg505 force-pushed the site-banner-height-fix branch from 24d9464 to 8f16896 Compare June 19, 2019 00:20
@cg505 cg505 requested a review from petemill June 19, 2019 00:26
@petemill
Copy link
Member

I'm not sure what causes the initial flash at the wrong size. If it's based on some provided height estimate, we can adjust that.

@cg505 I assume it's the difference between the non-loaded image and the loaded image, or the non-custom image and the custom image?

@cg505 cg505 force-pushed the site-banner-height-fix branch from 8f16896 to f9adb1f Compare June 19, 2019 18:49
@cg505 cg505 force-pushed the site-banner-height-fix branch from f9adb1f to dcb2cb2 Compare June 29, 2019 00:31
cg505 added 2 commits July 1, 2019 10:29
This avoids super tall tip dialogs while preserving the aspect ratio
of the banner images.
@cg505 cg505 force-pushed the site-banner-height-fix branch from dcb2cb2 to 5e1813e Compare July 1, 2019 17:30
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's check on any flashes of height when the dialog learns about the existence of a custom banner. If something is noticeable then we can follow-up.

@petemill
Copy link
Member

petemill commented Jul 1, 2019

Merging as previous build passed all checks. Most recent push was simply updating brave-ui after merge. https://staging.ci.brave.com/job/brave-core-build-pr/job/PR-2566/5/

@petemill petemill merged commit df6480b into master Jul 1, 2019
@petemill petemill deleted the site-banner-height-fix branch July 1, 2019 17:43
@bsclifton bsclifton removed this from the 0.68.x - Dev milestone Jul 1, 2019
@bsclifton bsclifton added this to the 0.69.x - Nightly milestone Jul 1, 2019
@bsclifton
Copy link
Member

Updated milestone to be master (0.69.x) since I didn't see this get uplifted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scale cover image height on tipping banner
4 participants