Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

[Bug] Add-ons Manager and add-on sub-pages have inconsistent style compared to the rest of Settings #8520

Closed
brampitoyo opened this issue Feb 19, 2020 · 5 comments

Comments

@brampitoyo
Copy link

brampitoyo commented Feb 19, 2020

Steps to reproduce

  • Go to Settings → Add-ons
  • Go to Settings → Add-ons → uBlock Origin

  1. Style/spacing of headers (“Recommended”, “Enabled”, “Disabled”) should be identical to style/spacing of Settings headers (“Account”, “General”, “Privacy and security”)

heading@2x


  1. Style/size/spacing of individual add-on name and description, should be identical to style/size/spacing of Site Permissions.

The colour of rating stars is already correct.

list-item@2x


  1. Style/size/spacing of add-on submenu items, should be identical to style/size/spacing of Settings list items.

sub-page@2x


  1. “On/Enabled” should be spaced so that its label aligns with item labels underneath.

ON@2x


  1. The icon used for “Permissions” under add-ons, should match the icon used for “Site Permissions” under Settings.

permissions-icon@2x


  1. Style/size/spacing used for “Details” and “Permissions”, should match style/size/spacing in “About Firefox”.
    • Remove underlines on links, and link icons to the right of “Homepage” and “Learn more about permissions”. We can use the same link colour as “About Firefox”, with no underline.
    • Colour/style of horizontal rulers should be identical to those found in “About Firefox”
    • “Details” already has the correct text-to-page margin of 16px. This value can be kept intact.
    • We can also keep using left-aligned text on both “Details” and “Permissions”, and keep the text regular-weight (some parts of “About Firefox” uses bold, and it’s not suitable for long body of information).

details-about

permissions-about@2x

┆Issue is synchronized with this Jira Task

@brampitoyo brampitoyo added 🐞 bug Crashes, Something isn't working, .. Feature:WebExtensions labels Feb 19, 2020
@brampitoyo brampitoyo self-assigned this Feb 19, 2020
@github-actions github-actions bot added the needs:triage Issue needs triage label Feb 19, 2020
@Amejia481 Amejia481 assigned Amejia481 and unassigned Amejia481 Feb 24, 2020
@Amejia481 Amejia481 self-assigned this Mar 9, 2020
@jonalmeida jonalmeida added this to ⏳ Sprint Backlog in A-C: Android Components Sprint Planning Mar 9, 2020
@Amejia481 Amejia481 moved this from ⏳ Sprint Backlog to 🏃‍♀️ In Progress in A-C: Android Components Sprint Planning Mar 25, 2020
Amejia481 added a commit to Amejia481/fenix that referenced this issue Mar 26, 2020
bors bot pushed a commit to mozilla-mobile/android-components that referenced this issue Mar 26, 2020
6406: Add api for customizing the ui of the AddonPermissionsAdapter and AddonsManagerAdapter r=psymoon a=Amejia481

Relate Fenix issue mozilla-mobile/fenix#8520



Co-authored-by: Arturo Mejia <arturomejiamarmol@gmail.com>
Amejia481 added a commit to Amejia481/fenix that referenced this issue Mar 30, 2020
Amejia481 added a commit to Amejia481/fenix that referenced this issue Mar 31, 2020
@Amejia481
Copy link
Contributor

@brampitoyo would you mind taking a look? This will be available in the next nightly update :)

@brampitoyo
Copy link
Author

brampitoyo commented Mar 31, 2020

@Amejia481 Thanks heaps for tackling this issue. I can confirm that our styles now matches Settings for the most part. Colours seem to be consistent 100% of the time, and text sizes are correct except in one instance.

The remaining tweaks have to do with paddings and element sizes.


  1. The header “Recommended”, “Enabled”, etc. needs to be shifted down a bit, so it matches the padding of headers like “Account” in Settings.

Just to check: are we also using the same font weight?


  1. Our add-on icon size is bigger than the size of favicons used elsewhere in the app, and our text padding doesn’t align with app bar header. Our text sizes are already correct, so that’s perfect.

If possible, we should follow the favicon specs outlined in #7992, where add-on icons is is 24x24dp centered in a 40x40dp box with 4dp corner radius. Sorry that it’s a moving target!


  1. On the “uBlock Origin” sub-page, the padding seems to be double the padding value of other Settings pages. Hopefully this is a fairly straightforward fix?


  1. On the “Details” sub-page, the text size (14dp?) is smaller than one found under “About Firefox” (16dp?). Our text colour now matches and looks great.


  1. If wonder if the vertical paddings of “Authors”, “Version”, “Last updated”, etc. can match the padding of items on the “Permissions” sub-page, and if the horizontal separator/ruler can stretch across the length of the page?

Both the horizontal paddings and text sizes are already correct.


6 On the “Permissions” sub-page, I wonder if we can stretch the horizontal separator/ruler across the length of the page? Similar to how rulers appear in other Settings page.

@brampitoyo
Copy link
Author

@Amejia481 All our remaining issues – some are issues from before – have to do with paddings between elements.

All of our text sizes and colours are now perfect!


  1. This was an issue from before. Our text padding doesn’t align with app bar header.

I was also wondering whether it would be possible to change the colour of the circle containing the add-on icon, to be the same colour as ones used on Bookmarks and History items?

  • Background colour value: Light Grey 30 #e0e0e6
  • No drop shadow

text-alignment@2x


  1. Our add/install (+) icon should have the same height (24dp), padding (16dp) and alignment as the three-dot icon used in Bookmarks and History.

icon-padding@2x


  1. The padding between add-ons header & list item should be the same as the padding between header & list item in Settings main page (I think it has no padding at all? I could be wrong).

heading-padding@2x


  1. On the “Details” sub-page, the top padding of add-on description should match text padding in other sections within Settings.

add-on-description-top-padding@2x


  1. On the “Details” sub-page, the padding between add-on description & details (authors, version, last updated, etc.) should be identical to the padding in the “About Firefox” page.

description-details-text-padding@2x


  1. This was an issue from before. On the “uBlock Origin” sub-page, the padding seems to be double the padding value of other Settings pages.

@brampitoyo brampitoyo added the eng:ready Ready for engineering label Apr 7, 2020
@brampitoyo brampitoyo removed their assignment Apr 7, 2020
@brampitoyo
Copy link
Author

@Amejia481 I’ve gone through all the changes and can confirm that nearly everything looks perfect. All major and minor layout issues have now been addressed. Thanks heaps for working on this!

There’s only one nit that remains: the plus icon we use isn’t the same as the plus icon used elsewhere in the app. Ours has flat/sharp edges, and the one used elsewhere has rounded edges. Ours is also smaller (20dp?) versus the other ones (24dp).

icon-size@2x

Otherwise, we’re ready to ship these changes!

Amejia481 added a commit to Amejia481/fenix that referenced this issue Apr 9, 2020
bors bot pushed a commit to mozilla-mobile/android-components that referenced this issue Apr 9, 2020
6602: Add api for customizing the style of the AddonPermissionsAdapter r=csadilek a=Amejia481

Related issue mozilla-mobile/fenix#8520 related Fenix pr mozilla-mobile/fenix#9833



Co-authored-by: Arturo Mejia <arturomejiamarmol@gmail.com>
@Amejia481 Amejia481 moved this from 🏃‍♀️ In Progress to 🚷 Blocked in A-C: Android Components Sprint Planning Apr 9, 2020
Amejia481 added a commit to Amejia481/fenix that referenced this issue Apr 13, 2020
Amejia481 added a commit that referenced this issue Apr 13, 2020
@liuche liuche mentioned this issue Apr 13, 2020
32 tasks
@Amejia481
Copy link
Contributor

The new changes are in nightly, we can close this one :)

A-C: Android Components Sprint Planning automation moved this from 🚷 Blocked to 🏁 Done Apr 15, 2020
@Amejia481 Amejia481 moved this from In progress to Done in A-C: WebExtensions and AddOns Apr 15, 2020
@ekager ekager removed the needs:triage Issue needs triage label Apr 17, 2020
@liuche liuche mentioned this issue Apr 28, 2020
32 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐞 bug Crashes, Something isn't working, .. eng:ready Ready for engineering Feature:WebExtensions implementation review
Projects
Development

No branches or pull requests

4 participants