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

Login Textfield Design #7971

Merged
merged 8 commits into from
Feb 12, 2021
Merged

Login Textfield Design #7971

merged 8 commits into from
Feb 12, 2021

Conversation

AndyScherzinger
Copy link
Member

@AndyScherzinger AndyScherzinger commented Feb 11, 2021

Using container-style for text fields (initially just for the login screen since it has a specific set of colors)

Also:

  • for testing updated notes dialog to native view binding
  • move login activity to native view binding

Screenshots (German)

Empty with hint Focused With Text
device-2021-02-11-124429 device-2021-02-11-124440 device-2021-02-11-124521

Sidenote

  • I am aware that there is also a design issue with the QR code part but that should go into a separate PR since it is not related to the changes needed for the text field.
  • The overall styling is "Material Standard" so we only changed the colors (as in "use white color"), all semi-transparencies, text sizes etc. are material's defaults.

Testing

  • Tests written, or not not needed

@AndyScherzinger
Copy link
Member Author

Waiting for the screenshot tests to update them accordingly...

Copy link
Member

@tobiasKaminsky tobiasKaminsky left a comment

Choose a reason for hiding this comment

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

Great job!

* utilize endIcon capabilities
* move to native view binding

Signed-off-by: Andy Scherzinger <[email protected]>
@AndyScherzinger
Copy link
Member Author

more screenshots for @jancborchardt

1 2 3 4
device-2021-02-11-150544 device-2021-02-11-150601 device-2021-02-11-150615 device-2021-02-11-150700

@jancborchardt
Copy link
Member

@AndyScherzinger thanks! Yeah, those warnings are there too – I would say it’s fine if below that warning container we have another block of text (which also shows when the warning is not there of course) saying:

The link to your Nextcloud web interface when you open it in the browser.

This can be in a bit subdued text, like 70% opacity white. We use that wording for the new desktop client flow as well: nextcloud/desktop#2895 (comment)

@AndyScherzinger
Copy link
Member Author

AndyScherzinger commented Feb 11, 2021

@jancborchardt see screenshots below. I put the error container below because there could actually be 2 lines so that would come with a large blue-space™️

Also "Nextcloud" is a placeholder, so the app name is brand-able like in other places throughout the client 👍

1 2 3
device-2021-02-11-180024 device-2021-02-11-180058 device-2021-02-11-180109

@codecov
Copy link

codecov bot commented Feb 11, 2021

Codecov Report

Merging #7971 (a2d31e8) into master (65300c3) will decrease coverage by 0.04%.
The diff coverage is 38.20%.

@@             Coverage Diff              @@
##             master    #7971      +/-   ##
============================================
- Coverage     29.27%   29.22%   -0.05%     
  Complexity        5        5              
============================================
  Files           443      443              
  Lines         34800    34804       +4     
  Branches       4809     4811       +2     
============================================
- Hits          10187    10172      -15     
- Misses        23076    23106      +30     
+ Partials       1537     1526      -11     
Impacted Files Coverage Δ Complexity Δ
...om/owncloud/android/ui/activity/EditorWebView.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...owncloud/android/ui/dialog/NoteDialogFragment.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../android/authentication/AuthenticatorActivity.java 37.74% <43.03%> (-0.24%) 0.00 <0.00> (ø)
.../java/com/nextcloud/client/jobs/OfflineSyncWork.kt 9.25% <0.00%> (-9.26%) 0.00% <0.00%> (ø%)
.../third_parties/daveKoeller/AlphanumComparator.java 79.76% <0.00%> (-2.39%) 0.00% <0.00%> (ø%)
.../owncloud/android/files/services/FileUploader.java 52.74% <0.00%> (-2.25%) 0.00% <0.00%> (ø%)
...va/com/owncloud/android/utils/FilesSyncHelper.java 21.33% <0.00%> (-1.34%) 0.00% <0.00%> (ø%)
...loud/android/datamodel/ThumbnailsCacheManager.java 37.45% <0.00%> (-1.26%) 0.00% <0.00%> (ø%)
...rc/main/java/com/owncloud/android/db/OCUpload.java 52.44% <0.00%> (-0.70%) 0.00% <0.00%> (ø%)
...owncloud/android/ui/adapter/OCFileListAdapter.java 39.25% <0.00%> (-0.66%) 0.00% <0.00%> (ø%)
... and 5 more

@jancborchardt
Copy link
Member

Noice @AndyScherzinger! Only design adjustment would be to make sure the text is flush left with the "Server address" field label and content, as well as having some more line-height, like so:
107670694-49dacb00-6c93-11eb-85e6-4c3092f75df9

* utilize endIcon capabilities
* move to native view binding

Signed-off-by: Andy Scherzinger <[email protected]>
@AndyScherzinger
Copy link
Member Author

AndyScherzinger commented Feb 11, 2021

@jancborchardt alignment fixed and line-multiplier=1.2 added 👍

1 2 3
device-2021-02-11-184143 device-2021-02-11-184206 device-2021-02-11-184217

@nextcloud-android-bot
Copy link
Collaborator

…ip ci]

Signed-off-by: drone <noreply@drone>
@nextcloud-android-bot
Copy link
Collaborator

@nextcloud-android-bot
Copy link
Collaborator

@nextcloud-android-bot
Copy link
Collaborator

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/7971.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

Codacy

Lint

TypemasterPR
Warnings243240
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings27
Correctness Warnings52
Internationalization Warnings9
Multithreaded correctness Warnings9
Performance Warnings73
Security Warnings41
Dodgy code Warnings99
Total310

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings27
Correctness Warnings52
Internationalization Warnings9
Multithreaded correctness Warnings9
Performance Warnings73
Security Warnings41
Dodgy code Warnings99
Total310

@nextcloud-android-bot
Copy link
Collaborator

White-Dark-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

@nextcloud-android-bot
Copy link
Collaborator

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Looks great design-wise, nice work @AndyScherzinger! :)

@nextcloud-android-bot
Copy link
Collaborator

White-Light-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

@nextcloud-android-bot
Copy link
Collaborator

@AndyScherzinger AndyScherzinger merged commit b760abd into master Feb 12, 2021
@delete-merged-branch delete-merged-branch bot deleted the textfieldDesign branch February 12, 2021 11:04
@nextcloud-android-bot
Copy link
Collaborator

@AndyScherzinger AndyScherzinger changed the title Textfield design Login Textfield designt Feb 15, 2021
@AndyScherzinger AndyScherzinger changed the title Login Textfield designt Login Textfield design Feb 15, 2021
@AndyScherzinger AndyScherzinger changed the title Login Textfield design Login Textfield Design Feb 15, 2021
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.

4 participants