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

Use material 3 style for SSO dialog #11001

Merged
merged 4 commits into from
Nov 8, 2022
Merged

Conversation

AlvaroBrey
Copy link
Member

@AlvaroBrey AlvaroBrey commented Nov 7, 2022

Before After
Screenshot_1667822281 Screenshot_1667822194

Signed-off-by: Álvaro Brey <alvaro.brey@nextcloud.com>
Can't use a compound drawable here, we need to size the image

Signed-off-by: Álvaro Brey <alvaro.brey@nextcloud.com>
@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #11001 (d56026f) into master (fb52b3f) will increase coverage by 28.08%.
The diff coverage is 0.00%.

❗ Current head d56026f differs from pull request most recent head 6810540. Consider uploading reports for the commit 6810540 to get more accurate results

Additional details and impacted files
@@              Coverage Diff              @@
##             master   #11001       +/-   ##
=============================================
+ Coverage      3.23%   31.32%   +28.08%     
- Complexity      420     3273     +2853     
=============================================
  Files           544      544               
  Lines         40997    41008       +11     
  Branches       5681     5681               
=============================================
+ Hits           1328    12847    +11519     
+ Misses        39588    26245    -13343     
- Partials         81     1916     +1835     
Impacted Files Coverage Δ
...ndroid/ui/activity/SsoGrantPermissionActivity.java 1.17% <0.00%> (+1.17%) ⬆️
.../third_parties/daveKoeller/AlphanumComparator.java 86.90% <0.00%> (-1.20%) ⬇️
.../java/com/owncloud/android/datamodel/GalleryRow.kt 50.00% <0.00%> (ø)
...roid/ui/activity/ReceiveExternalFilesActivity.java 0.36% <0.00%> (+0.36%) ⬆️
...owncloud/android/ui/activity/PassCodeActivity.java 0.51% <0.00%> (+0.51%) ⬆️
...cloud/android/ui/preview/PreviewImageActivity.java 0.55% <0.00%> (+0.55%) ⬆️
...owncloud/android/ui/activity/UserInfoActivity.java 0.58% <0.00%> (+0.58%) ⬆️
...oud/android/ui/activity/NotificationsActivity.java 0.59% <0.00%> (+0.59%) ⬆️
...wncloud/android/ui/preview/PreviewVideoActivity.kt 0.94% <0.00%> (+0.94%) ⬆️
...ui/fragment/contactsbackup/BackupListFragment.java 0.94% <0.00%> (+0.94%) ⬆️
... and 274 more

Copy link
Member

@AndyScherzinger AndyScherzinger left a comment

Choose a reason for hiding this comment

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

@AlvaroBrey the buttons/background(?) need theming after .show()

@AlvaroBrey
Copy link
Member Author

@AlvaroBrey the buttons/background(?) need theming after .show()

I'm not sure it makes sense; as this activity may be called asking for permission for an user that's not the currently active user.

@stefan-niedermann
Copy link
Member

Regarding theming I'd recommend white / black text for thr buttons. The default Material You color is not used anywhere I am aware of...

@AndyScherzinger
Copy link
Member

I'm not sure it makes sense; as this activity may be called asking for permission for an user that's not the currently active user.

True while I would argue to either go with @stefan-niedermann suggestions to go with a fall back of black/white or we could just use a color based on the xml-values? In any case any color other the material's default violet would be good I think.

@AlvaroBrey
Copy link
Member Author

What about R.color.primary (passed through m3 utils)? This way it's not as boring as white but it's still on-brand.

Dark Light
Screenshot_1667831290 Screenshot_1667831496

…let color

Signed-off-by: Álvaro Brey <alvaro.brey@nextcloud.com>
@AndyScherzinger
Copy link
Member

What about R.color.primary (passed through m3 utils)? This way it's not as boring as white but it's still on-brand.

@AlvaroBrey perfect, let's go with that, also did you check on background theming with surface ? I think we do this in other places like:

viewThemeUtils.dialog.colorMaterialAlertDialogBackground(
                    binding.pollVoteEndPollButton.context,
                    dialogBuilder
                )

might be tricky though since this doesn't have a color pass-through variant

Signed-off-by: Álvaro Brey <alvaro.brey@nextcloud.com>
@AlvaroBrey
Copy link
Member Author

did you check on background theming with surface

Done. The only difference is that the background for the dark variant is a little darker (I guess for better contrast with the buttons)

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

Codacy

Lint

TypemasterPR
Warnings8383
Errors00

SpotBugs

CategoryBaseNew
Bad practice2828
Correctness4545
Dodgy code341341
Internationalization99
Multithreaded correctness99
Performance5858
Security2828
Total518518

@github-actions
Copy link

github-actions bot commented Nov 8, 2022

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/11001.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@nextcloud-android-bot
Copy link
Collaborator

@AndyScherzinger AndyScherzinger merged commit ae802cd into master Nov 8, 2022
@delete-merged-branch delete-merged-branch bot deleted the fix/sso-dialog-theming branch November 8, 2022 09:25
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.23.0 milestone Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to Material 3 theme
4 participants