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 3️⃣ alignment with Files and Talk #1648

Closed
AndyScherzinger opened this issue Jan 24, 2023 · 9 comments
Closed

Material 3️⃣ alignment with Files and Talk #1648

AndyScherzinger opened this issue Jan 24, 2023 · 9 comments
Labels

Comments

@AndyScherzinger
Copy link
Member

Just to have the discussion, looping in @tobiasKaminsky and @stefan-niedermann what do you think? Should be adapt the styling we put in place for files and talk which I think would basically translate to the top bar(s) and FAB at this point? Can't tell about check-boxes etc. since I didn't take a look at the code / theming yet. First wanted to see if this would even be something to consider before digging deeper and maybe working on a PR. In a way my assumption is it hasn't been put into place to give notes a completely flat UI to mimic paper? (Wild guess, because if Stefan would have wanted it and had the time (don't know) I am sure he would have implemented it already since the UI is slick!)

Looking forward to your comments 💙

Also @jancborchardt and @nimishavijay for your opinion

@AndyScherzinger AndyScherzinger changed the title Material 3 Alignment with Files and Talk Material 3️⃣ alignment with Files and Talk Jan 24, 2023
@stefan-niedermann
Copy link
Member

What do you have specifically in mind? The Notes Android app already uses the Material 3 themes 🙂

@tobiasKaminsky
Copy link
Member

I am all in for alignment, especially code wise, so that we can e.g. also use server/user based theming at some point.
But it has low priority, as app looks already great!

@stefan-niedermann
Copy link
Member

use server/user based theming at some point.

I still can't follow you guys, the server theme color is already applied 🙈 - code and optical alignments make sense of course

@tobiasKaminsky
Copy link
Member

Sorry!
We want to extract theming/color stuff from Talk and Files to android-common and already started with it.
And I think it is best to have then the same code used in Notes.

@AndyScherzinger
Copy link
Member Author

AndyScherzinger commented Jan 26, 2023

Some comparison shots, didn't yet look at the code but running the app against cloud.nextcloud shows the differences to me:

notes files
214859950-b726f827-d766-4f5c-8be7-9d8511c503ac Screenshot_20230125_095346
Screenshot_20230126_152233 Screenshot_20230125_095433
Screenshot_20230126_152226 Screenshot_20230126_152126

So both apps use the m3 theme, code-wise while the colors on notes are still m2-based. For files and talk this is "fixed" because we ship Google's M3 color calculations and wrapped them in theme-able functions calculating any color based on M3 and applying it accordingly. It is more obvious in some cases and very, very subtle in others. Basically M3 uses a shade of the primary color on all colors and also recalculates primary for dark/light with accessibility and lightning in mind, hence the more obvious color differences. Other things are:

  • Toolbar is slightly tainted primary (last screenshots), same for the toolbar icons
  • Main menu tins the active item's background with a primary shade, the rounded corners are missing on files (yet)
  • Search bar is colored and same for hint text, icon size/color
  • the primary color change would apply to any primary colored element like buttons and check-boxes

Hope this sheds some light on what I try to discuss here. Implementation would be based on the common lib that has been build for Nextcloud Android apps.

@AndyScherzinger
Copy link
Member Author

So I digged a little bit into the code and it seems it would be relatively tricky to get the implementation in because it is based on a set of helper classes injected via dagger being setup once and then used everywhere until user-context switch and re-initiated while Notes uses a live data object and else is based on the stored primary color. Having a hard time to wrap my head around how to move to the theming implementation we use from Nextclouds common lib. Not sure, maybe @AlvaroBrey has an idea?

@AlvaroBrey
Copy link
Member

So I digged a little bit into the code and it seems it would be relatively tricky to get the implementation in because it is based on a set of helper classes injected via dagger being setup once and then used everywhere until user-context switch and re-initiated while Notes uses a live data object and else is based on the stored primary color. Having a hard time to wrap my head around how to move to the theming implementation we use from Nextclouds common lib. Not sure, maybe @AlvaroBrey has an idea?

I can have a look once #1644 and #1645 are done, which are the current priorities for this app; I assume I'll also get some insight while working on those.

AlvaroBrey added a commit that referenced this issue Feb 16, 2023
feat(theming): Use nextcloud-common library for theming UI elements (#1648)
@stefan-niedermann
Copy link
Member

I think we can close this issue now as the most visible parts have been migrated to android-common. Instead let's open separate issues if we find more places where theming needs to get applied properly.

@AndyScherzinger @tobiasKaminsky ?

@AndyScherzinger
Copy link
Member Author

Yeah, makes sense 👍

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

No branches or pull requests

4 participants