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

Material paper drawer background #3851

Merged
merged 1 commit into from
Jan 6, 2019
Merged

Conversation

ByteHamster
Copy link
Collaborator

The background even works well with yellow. The account chip still looks strange when selecting yellow: restricting the account colors to not allow yellow will be done in another PR.


@ByteHamster
Copy link
Collaborator Author

By the way, the image is not repeatable. It looks too symmetric if it's repeatable, in my opinion.

@cketti
Copy link
Member

cketti commented Dec 29, 2018

@ByteHamster: Can you please create the PNGs for the different density buckets we support for regular drawables (mdpi tp xxhdpi).

@ByteHamster
Copy link
Collaborator Author

I think it would look good to have the sidebar menu go below the status bar like in most Google apps. What do you think about that?

@cketti
Copy link
Member

cketti commented Dec 29, 2018

I think it would look good to have the sidebar menu go below the status bar like in most Google apps. What do you think about that?

👍

@Dafnik
Copy link

Dafnik commented Dec 29, 2018

No most google apps don't do this. It was a thing when the first version of Material design was released.

@ByteHamster
Copy link
Collaborator Author

Hi @Dafnik, not sure if we are talking about the same thing. The Google apps I use (Calendar, Gmail, Play store, Play console, Translate) all display the sidebar below the status bar. Only Google Maps does not display the sidebar below the status bar.

screenshot_20181229-234822_google_play_store-picsay

@Dafnik
Copy link

Dafnik commented Dec 30, 2018

Oh I'm sorry, I thought you talked about the toolbar. My fault 👍

generate hdpi 576 324
generate xhdpi 768 432
generate xxhdpi 1152 648
#generate xxxhdpi 1728 972
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented out? Not used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cketty told me to support mdpi tp xxhdpi. Just in case we want to support xxxhdpi sometimes, there sizes are already here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Err, sounds legit. And it's not commented-out because the file generated would be big or for what reason?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I did not want to add more big png files. I don't know what cketty thinks about that

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, for me it's OK just wanted to clarify, thanks 👍

Copy link
Member

Choose a reason for hiding this comment

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

xxxhdpi is ridiculously dense and really should only be used for launcher icons because "some app launchers display your app icon as much as 25% larger than what's called for by the device's density bucket". (https://developer.android.com/training/multiscreen/screendensities)

See also https://developer.android.com/guide/topics/resources/providing-resources

@wiktor-k wiktor-k self-requested a review January 2, 2019 12:12
Copy link
Contributor

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

This looks very good on a real device 👍

@cketti cketti merged commit 6a64d5e into thunderbird:master Jan 6, 2019
@cketti
Copy link
Member

cketti commented Jan 6, 2019

👍

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

Successfully merging this pull request may close these issues.

4 participants