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

Brave Ads notification icon is too small and almost invisible #2887

Closed
srirambv opened this issue Jan 10, 2019 · 16 comments
Closed

Brave Ads notification icon is too small and almost invisible #2887

srirambv opened this issue Jan 10, 2019 · 16 comments

Comments

@srirambv
Copy link
Contributor

Description

Notification icon is too small and almost invisible

Steps to Reproduce

  1. Clean install 0.60.9
  2. Enable rewards and ads
  3. Wait till an ad notification comes though
  4. Icon is too small on the notification

Actual result:

Icon on the notification is too small. Might need an appointment with ophthalmologist

Expected result:

No doctor appointment needed (unless really required) and icons should be large enough to fit in the ad notification

Reproduces how often:

Easy

Brave version (brave://version info)

Brave 0.60.9 Chromium: 72.0.3626.28 (Official Build) dev(64-bit)
Revision 997b1040b63bac324e815797ba52be0cd8f616ed-refs/branch-heads/3626@{#461}
OS All

Reproducible on current release:

  • Does it reproduce on brave-browser dev/beta builds?
    Yes on dev build

Website problems only:

  • Does the issue resolve itself when disabling Brave Shields?
    No
  • Is the issue reproducible on the latest version of Chrome?
    No

Additional Information

cc: @jsecretan @bridiver @brave/legacy_qa

@bridiver
Copy link
Contributor

Too small based on what? The spec only shows macos notifications and the icons match the specs afaik. On some (maybe all) platforms we have no control over this anyway.

@srirambv
Copy link
Contributor Author

This is how it looks on Windows. And this is a zoomed in version of the print screen.
image

@bridiver
Copy link
Contributor

can you take this to product? There is nothing in the spec for windows and I'm not sure if it's even possible to control the size of the icon or not on windows. Either way strictly speaking this isn't a bug, it's feature request.

@kjozwiak
Copy link
Member

@bridiver do you know who from the product side is currently working on ads? CCing @bradleyrichter.

@rebron rebron removed this from the 1.x Backlog milestone Feb 7, 2019
@tmancey
Copy link
Contributor

tmancey commented Feb 15, 2019

@bridiver @kjozwiak With regards to icon size, this is not possible on macOS.

@mandar @kjozwiak Mandar is working on Ads

@kjozwiak
Copy link
Member

@srirambv mind adding a screenshot/example of the above for Windows so @mandar-brave can visually see the issue and if we need to improve the icon sizes?

@bridiver @kjozwiak With regards to icon size, this is not possible on macOS.

Looks like this isn't possible on macOS as per @tmancey so added the OS/Linux and OS/Window flags.

@mandar-brave
Copy link

thanks @kjozwiak
Never seen the issue at my end on Windows to-date so must be related to some hi-res screen needs. But waiting to see screenshots and will do needful.

@srirambv
Copy link
Contributor Author

@kjozwiak @mandar-brave screenshot is added here #2887 (comment). You can see how small the Brave icon is compared to the size of the notification. This is on 125% DPI default setting on a 1920x1080(16:9) display. Pretty sure as the DPI increases the notification size increases proportionately but not the Brave icon

@mandar-brave
Copy link

@tmancey @bridiver is there a limitation with windows API? I am very sure this is not an issue with Mac as such but checking in; we can expect the image to scale proportionately as well. Thanks

@srirambv
Copy link
Contributor Author

The spec only shows macos notifications and the icons match the specs afaik

Specs should contain behaviour on all platforms and not just macOS so that these issues can be ironed out early on

@mandar-brave
Copy link

@srirambv couple things - we have been relegated to use an existing API so the spec does not "say what OS" - it is suggesting format. We are not developing a custom notification if you read the spec - we are going to use a standard chrome API that is available to us. Same will happen to local notifications on Android. The entire idea = use local system notifications.

@jsecretan the UX here will look horrible for high DIP issues; upgrading to P2 for launch

@mandar-brave mandar-brave added the priority/P2 A bad problem. We might uplift this to the next planned release. label Feb 26, 2019
@jonathansampson
Copy link
Contributor

Came here to say the same thing. Ad icons are too small on Windows. For comparison, here's a 160x160 image in a Web Notification (right) next to one of our ads today:

image

@srirambv
Copy link
Contributor Author

srirambv commented Apr 4, 2019

Small icon issue on Linux as well. Compared to the size of the notification the icon is too small
Screenshot from 2019-04-04 16-19-45

@tmancey
Copy link
Contributor

tmancey commented Apr 19, 2019

DEVELOPER NOTE: See enable_hidpi in codebase

@jsecretan jsecretan added priority/P3 The next thing for us to work on. It'll ride the trains. and removed priority/P2 A bad problem. We might uplift this to the next planned release. labels Jul 17, 2019
@tmancey tmancey changed the title Ad notification icon is too small and almost invisible Brave Ad notification icon is too small and almost invisible Jan 20, 2020
@tmancey tmancey removed QA/Test-Plan-Specified QA/Yes priority/P3 The next thing for us to work on. It'll ride the trains. labels Jan 20, 2020
@tmancey tmancey changed the title Brave Ad notification icon is too small and almost invisible Brave Ads notification icon is too small and almost invisible Jan 20, 2020
@tmancey
Copy link
Contributor

tmancey commented May 29, 2020

Blocked as issue will be closed once #9592 has been implemented

@tmancey
Copy link
Contributor

tmancey commented Oct 22, 2020

Closing issue as superseded by #9592

@tmancey tmancey closed this as completed Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

8 participants