For #7836: Refine add-ons badge design #7838

hkaancaliskan · 2020-07-24T00:21:49Z

cc @brampitoyo

mozilla-mobile/fenix#12845 (comment)

Colors are coming from extensions. They need to set more stylish colors.

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

cadeyrn · 2020-07-25T07:29:49Z

Thanks for working on it. Apart from the colours I noticed some other things that are different from the mockup:

The width is different (width should be equal to height, in your screenshot it has a bigger width than height).
The border radius is different (your screenshot seems to have a larger border radius than the mockup).
The font weight is different (it should be bold, it's not bold in your screenshot).
The font size is different (not a lot, but it's a bit bigger in your screenshot compared to the mockup).

edit 2020/07/27: feedback is based on an older screenshot that is no longer visible in comment #0.

hkaancaliskan · 2020-07-25T08:09:52Z

Thanks for working on it. Apart from the colours I noticed some other things that are different from the mockup:

The width is different (width should be equal to height, in your screenshot it has a bigger width than height).

The border radius is different (your screenshot seems to have a larger border radius than the mockup).

The font weight is different (it should be bold, it's not bold in your screenshot).

The font size is different (not a lot, but it's a bit bigger in your screenshot compared to the mockup).

Thanks for feedback. That's why this PR is draft :)
Actually I'm waiting for a ux mock to see how exactly it should be @brampitoyo
(Not image, the ones that i can see margins, radius, height width etc. ones)

hkaancaliskan · 2020-07-25T17:13:37Z

When i run ./gradlew test i get unrelated test fails and can't get menu fail.

This pr test result: pr.zip

Master test result: master.zip

Amejia481 · 2020-07-27T18:55:35Z

In my case, I prefer only to run tests for the module that I'm modifying in the moment and open a pull request to let the CI run all the rest. For running test for browser-menu will be only ./gradlew -Pcoverage browser-menu:test

It looks the only tests that are failing are related to browser-menu it could linked to the background.setTint(it) change.

You can see the logs for an module just by clicking the module name build-browser-menu -> Details -> View task in Taskcluster -> View Live Log

browser-menu logs

...owser/menu/src/main/java/mozilla/components/browser/menu/item/WebExtensionBrowserMenuItem.kt

hkaancaliskan · 2020-07-27T21:35:24Z

@Amejia481 sorry but I'm unable to figure out what is wrong with test. I'm not good with tests actually. Why does action.badgeBackgroundColor?.let { badgeView.background.setTint(it) } throwing null pointer?

java.lang.NullPointerException
	at mozilla.components.browser.menu.item.WebExtensionBrowserMenuItem.bind(WebExtensionBrowserMenuItem.kt:57)
	at mozilla.components.browser.menu.item.WebExtensionBrowserMenuItemTest.badge text view is invisible if action badge text is empty(WebExtensionBrowserMenuItemTest.kt:157)

Amejia481 · 2020-07-28T18:16:39Z

@Amejia481 sorry but I'm unable to figure out what is wrong with test. I'm not good with tests actually. Why does action.badgeBackgroundColor?.let { badgeView.background.setTint(it) } throwing null pointer?
java.lang.NullPointerException
	at mozilla.components.browser.menu.item.WebExtensionBrowserMenuItem.bind(WebExtensionBrowserMenuItem.kt:57)
	at mozilla.components.browser.menu.item.WebExtensionBrowserMenuItemTest.badge text view is invisible if action badge text is empty(WebExtensionBrowserMenuItemTest.kt:157)

@hakkikaancaliskan I think it's failing because we are now calling badgeView.background and background could be null that it's the case of the tests. If we call badgeView.background?.setTint(it) instead, it will be safer as we are not assuming that we always have a background :)

...owser/menu/src/main/java/mozilla/components/browser/menu/item/WebExtensionBrowserMenuItem.kt

...r/menu/src/test/java/mozilla/components/browser/menu/item/WebExtensionBrowserMenuItemTest.kt

hkaancaliskan · 2020-07-28T21:51:52Z

I think it's done. Could you please check?
If ok, I'm gonna squash commits.

Amejia481 · 2020-07-28T21:54:51Z

The code looks good to me :) Let's see what the CI says XD
Also we need the feedback from @brampitoyo before landing, that final touch will be to squash all the commits into a single one

hkaancaliskan · 2020-07-28T21:59:03Z

The code looks good to me :) Let's see what the CI says XD
Also we need the feedback from @brampitoyo before landing, that final touch will be to squash all the commits into a single one

👍
I hate tests xd

Amejia481 · 2020-07-28T22:01:37Z

The code looks good to me :) Let's see what the CI says XD
Also we need the feedback from @brampitoyo before landing, that final touch will be to squash all the commits into a single one

👍
I hate tests xd

Nobody love them but after a while you get used to have a good relationship 😉

brampitoyo · 2020-07-29T03:20:34Z

@hakkikaancaliskan I’ve posted the badge specs on mozilla-mobile/fenix#12845 (comment)

hkaancaliskan · 2020-08-03T07:12:03Z

@Amejia481 mozilla-mobile/fenix#12845 (comment)
UX approved :)

hkaancaliskan · 2020-08-08T14:55:45Z

@Amejia481 beep beep

Amejia481 · 2020-08-09T14:09:27Z

Sorry for the delay, great job!

Amejia481

bors r+

@brampitoyo

7838: For #7836: Refine add-ons badge design r=Amejia481 a=hakkikaancaliskan cc @brampitoyo mozilla-mobile/fenix#12845 (comment) Colors are coming from extensions. They need to set more stylish colors. <img src="https://user-images.githubusercontent.com/17825767/88461470-61e02e00-ceac-11ea-99a4-822e3bc05d8f.png" width="200"><img src="https://user-images.githubusercontent.com/17825767/88461472-63115b00-ceac-11ea-968e-3cee2eea8d7f.png" width="200"><img src="https://user-images.githubusercontent.com/17825767/88461474-63115b00-ceac-11ea-90a8-ba043509c1e6.png" width="200">  ### Pull Request checklist  - [x] **Quality**: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended) - [x] **Tests**: This PR includes thorough tests or an explanation of why it does not - [x] **Changelog**: This PR includes [a changelog entry](https://github.com/mozilla-mobile/android-components/blob/master/docs/changelog.md) or does not need one - [x] **Accessibility**: The code in this PR follows [accessibility best practices](https://github.com/mozilla-mobile/shared-docs/blob/master/android/accessibility_guide.md) or does not include any user facing features ### After merge - **Milestone**: Make sure issues closed by this pull request are added to the [milestone](https://github.com/mozilla-mobile/android-components/milestones) of the version currently in development. - **Breaking Changes**: If this is a breaking change, please push a draft PR on [Reference Browser](https://github.com/mozilla-mobile/reference-browser) to address the breaking issues. Co-authored-by: Hakkı Kaan Çalışkan <caliskanhkaan@gmail.com>

hkaancaliskan · 2020-08-09T14:11:39Z

Sorry for the delay, great job!

No problem :) squash commits needed?

hkaancaliskan · 2020-08-09T14:13:38Z

Sorry for the delay, great job!

Oops, you guys working on Sunday too?

Amejia481 · 2020-08-09T14:14:34Z

bors r-

bors · 2020-08-09T14:14:35Z

Canceled.

Amejia481 · 2020-08-09T14:15:18Z

Sorry for the delay, great job!

No problem :) squash commits needed?

Yes, please :)

Amejia481 · 2020-08-09T14:17:26Z

Sorry for the delay, great job!

Oops, you guys working on Sunday too?

Not but I felt guilty as didn't review before 😅, do not worries it's not a problem.

hkaancaliskan · 2020-08-09T14:18:50Z

Sorry for the delay, great job!

Oops, you guys working on Sunday too?

Not but I felt guilty as didn't review before 😅, do not worries it's not a problem.

Ohh of course this could wait monday. Go bro go, enjoy weekend 😂

Amejia481 · 2020-08-09T14:24:43Z

Ohh of course this could wait monday. Go bro go, enjoy weekend 😂

Thanks :), really don't worry after squashing the commit it will be just landing :)

hkaancaliskan · 2020-08-09T14:30:03Z

Ohh of course this could wait monday. Go bro go, enjoy weekend 😂

Thanks :), really don't worry after squashing the commit it will be just landing :)

Squashed and rebased 😊

Amejia481 · 2020-08-09T14:30:27Z

bors r+

Amejia481 · 2020-08-09T14:31:15Z

Ohh of course this could wait monday. Go bro go, enjoy weekend 😂

Thanks :), really don't worry after squashing the commit it will be just landing :)

Squashed and rebased 😊

Thanks :)
Hope you have a great rest of the weekend!

hkaancaliskan · 2020-08-09T14:31:42Z

Ohh of course this could wait monday. Go bro go, enjoy weekend 😂

Thanks :), really don't worry after squashing the commit it will be just landing :)

Squashed and rebased 😊

Thanks :)
Hope you have a great rest of the weekend!

Hope you too :)

bors · 2020-08-09T14:58:28Z

Build succeeded:

complete-push

Amejia481 self-requested a review July 27, 2020 14:09

Amejia481 self-assigned this Jul 27, 2020

Amejia481 added the 🕵️‍♀️ needs review PRs that need to be reviewed label Jul 27, 2020

Amejia481 reviewed Jul 27, 2020

View reviewed changes

...owser/menu/src/main/java/mozilla/components/browser/menu/item/WebExtensionBrowserMenuItem.kt Show resolved Hide resolved

hkaancaliskan marked this pull request as ready for review July 27, 2020 21:35

Amejia481 reviewed Jul 28, 2020

View reviewed changes

...owser/menu/src/main/java/mozilla/components/browser/menu/item/WebExtensionBrowserMenuItem.kt Outdated Show resolved Hide resolved

Amejia481 reviewed Jul 28, 2020

View reviewed changes

...r/menu/src/test/java/mozilla/components/browser/menu/item/WebExtensionBrowserMenuItemTest.kt Outdated Show resolved Hide resolved

Amejia481 reviewed Jul 28, 2020

View reviewed changes

...r/menu/src/test/java/mozilla/components/browser/menu/item/WebExtensionBrowserMenuItemTest.kt Outdated Show resolved Hide resolved

Amejia481 reviewed Jul 28, 2020

View reviewed changes

...r/menu/src/test/java/mozilla/components/browser/menu/item/WebExtensionBrowserMenuItemTest.kt Show resolved Hide resolved

hkaancaliskan mentioned this pull request Jul 30, 2020

Refine add-ons badge design mozilla-mobile/fenix#12845

Closed

Amejia481 self-requested a review August 9, 2020 14:09

Amejia481 approved these changes Aug 9, 2020

View reviewed changes

Amejia481 added 🛬 needs landing PRs that are ready to land and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Aug 9, 2020

For #7836: Refine add-ons badge design

59aa1bb

bors bot merged commit 0ea5c6f into mozilla-mobile:master Aug 9, 2020

hkaancaliskan deleted the addon-badge-design branch August 12, 2020 10:56

For #7836: Refine add-ons badge design #7838

For #7836: Refine add-ons badge design #7838

Uh oh!

Conversation

hkaancaliskan commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request checklist

After merge

Uh oh!

cadeyrn commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hkaancaliskan commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hkaancaliskan commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amejia481 commented Jul 27, 2020

Uh oh!

Uh oh!

hkaancaliskan commented Jul 27, 2020

Uh oh!

Amejia481 commented Jul 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hkaancaliskan commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amejia481 commented Jul 28, 2020

Uh oh!

hkaancaliskan commented Jul 28, 2020

Uh oh!

Amejia481 commented Jul 28, 2020

Uh oh!

brampitoyo commented Jul 29, 2020

Uh oh!

hkaancaliskan commented Aug 3, 2020

Uh oh!

hkaancaliskan commented Aug 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amejia481 commented Aug 9, 2020

Uh oh!

Amejia481 left a comment

Choose a reason for hiding this comment

Uh oh!

hkaancaliskan commented Aug 9, 2020

Uh oh!

hkaancaliskan commented Aug 9, 2020

Uh oh!

Amejia481 commented Aug 9, 2020

Uh oh!

bors bot commented Aug 9, 2020

Uh oh!

Amejia481 commented Aug 9, 2020

Uh oh!

Amejia481 commented Aug 9, 2020

Uh oh!

hkaancaliskan commented Aug 9, 2020

Uh oh!

Amejia481 commented Aug 9, 2020

Uh oh!

hkaancaliskan commented Aug 9, 2020

Uh oh!

Amejia481 commented Aug 9, 2020

Uh oh!

Amejia481 commented Aug 9, 2020

Uh oh!

hkaancaliskan commented Aug 9, 2020

Uh oh!

bors bot commented Aug 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

hkaancaliskan commented Jul 24, 2020 •

edited

Loading

cadeyrn commented Jul 25, 2020 •

edited

Loading

hkaancaliskan commented Jul 25, 2020 •

edited

Loading

hkaancaliskan commented Jul 25, 2020 •

edited

Loading

hkaancaliskan commented Jul 28, 2020 •

edited

Loading

hkaancaliskan commented Aug 8, 2020 •

edited

Loading