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

Added new restore icon (to replace the small one) #4865

Merged

Conversation

Kilian-Perisset
Copy link
Member

Fixing issue : #4848

@tobiasKaminsky
Copy link
Member

Once you think the PR is good for review, please add "2. to review" as label.

@tobiasKaminsky
Copy link
Member

As you are a member, you can also push a branch / PR directly to our nextcloud repo.
This makes reviewing a bit easier for the other contributors :-)

@tobiasKaminsky
Copy link
Member

Thank you for your PR.
How it looks like:
image

How the restore button of web ui looks like:
image

How it previously looked:
image

Can you try to re-use the old icon, but make it bigger?
Original svg logo can be found here: https://github.com/nextcloud/android/blob/f8801115f2f9a27b6da52aaf5b1c36940b1f2652/drawable_resources/history.svg

@Kilian-Perisset
Copy link
Member Author

@tobiasKaminsky

Thanks for these advices and guidelines
In my opinion, I find the icon too "bold" :

Screenshot_20191121-174622_Nextcloud

But I may apply this one if you want to.

@AndyScherzinger
Copy link
Member

@jancborchardt can you comment on the @nextcloud/designers preference regarding the icon? (Last two comments). Thanks ❤️

@Kilian-Perisset
Copy link
Member Author

@AndyScherzinger @tobiasKaminsky
It would be a pleasure to push on your repository, except that I don't really understand the relationship between the guidelines and the current situation of the repo branches.

I'm very motivated to help you improve this application, but these branches don't really motivate to do the things properly (since the order is not very present)

Don't you want help to clean up the branches and issues so that we can have a proper and workeable project to work on ?

@jancborchardt
Copy link
Member

Yep, the current icon is indeed too thick, thanks for looking into that @Shagequi

Is this a Material design icon? If not, can we pick one from Material icons?
Cause it seems a bit big and thin now, and the arrow is not so visible.
And then could you also do a pull request for server to replace the icon in core/img/actions so it is consistent – thanks a lot! :)

@Shagequi regarding branches:
You are a member of the Nextcloud github org, so you do not need a fork. Simply create a branch on this repository, like you did on your fork.
That way if changes are needed, other collaborators can do those too.
Master is the main branch and everything else are feature branches (pull requests). Do you mean there are a lot of outdates branches lying around which should be cleaned up?

@jancborchardt
Copy link
Member

(This is off-topic, but to clean up branches, a good starting point is to go through the stale branches and delete those not needed anymore: https://github.com/nextcloud/android/branches/stale :)

@tobiasKaminsky
Copy link
Member

Is this a Material design icon? If not, can we pick one from Material icons?

I thought I picked it up from server repo? 🤔

@tobiasKaminsky
Copy link
Member

@Shagequi let us have this discussion in a new ticket please.
@jancborchardt you should know it better and not start off topic here!

@AndyScherzinger
Copy link
Member

AndyScherzinger commented Nov 22, 2019

Is this a Material design icon? If not, can we pick one from Material icons?

It is looking like one (but isn't), while I would suggest:
https://materialdesignicons.com/icon/replay
replay

What do you think @jancborchardt @tobiasKaminsky @Shagequi ?

I thought I picked it up from server repo? 🤔

You did, and the server's icon is not a material design icon

@Kilian-Perisset
Copy link
Member Author

@AndyScherzinger I approve this icon, more visible :)

@tobiasKaminsky
Copy link
Member

I am fine with this material icon, but what about @jancborchardt ?

@jancborchardt
Copy link
Member

Yes, the icon was correctly picked from the server repo. :) However @Shagequi is right that the server icon is too bold. Which is why I said above:

And then could you also do a pull request for server to replace the icon in core/img/actions so it is consistent – thanks a lot! :)

And yes, the Material replay icon looks good!

@tobiasKaminsky
Copy link
Member

@Shagequi is this really the material icon? For me it looks somehow the same like the old one…?

@nextcloud-android-bot
Copy link
Collaborator

@Kilian-Perisset
Copy link
Member Author

@tobiasKaminsky

Here is the difference between the two icons :

NextCloud :
Screenshot_20191125-145735_Nextcloud

My version :
Screenshot_20191125-145746_kDrive

Unless I'm mistaken, the one I propose has been redesigned by one of our UI designer.
So it's based on Material.

@tobiasKaminsky
Copy link
Member

It is looking like one (but isn't), while I would suggest:
https://materialdesignicons.com/icon/replay
replay

What do you think @jancborchardt @tobiasKaminsky @Shagequi ?

I thought I picked it up from server repo? thinking

You did, and the server's icon is not a material design icon

Andy suggested the one by Android's material design and Jan approved it, so please use this one.

@AndyScherzinger
Copy link
Member

Andy suggested the one by Android's material design and Jan approved it, so please use this one.

@Shagequi would yoou replace it within this PR or should I? (fine either way) 😃

@Kilian-Perisset
Copy link
Member Author

@AndyScherzinger Yes I will replace the icon today :)

@AndyScherzinger
Copy link
Member

celebrate

Copy link
Member Author

@Kilian-Perisset Kilian-Perisset left a comment

Choose a reason for hiding this comment

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

The branch has been updated

@nextcloud-android-bot
Copy link
Collaborator

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

333

Lint

TypemasterPR
Warnings5959
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings70
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings104
Security Warnings44
Dodgy code Warnings136
Total407

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings70
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings104
Security Warnings44
Dodgy code Warnings136
Total407

@codecov
Copy link

codecov bot commented Nov 28, 2019

Codecov Report

Merging #4865 into master will increase coverage by 0.09%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #4865      +/-   ##
============================================
+ Coverage      17.6%   17.69%   +0.09%     
  Complexity        3        3              
============================================
  Files           377      384       +7     
  Lines         32403    32650     +247     
  Branches       4580     4597      +17     
============================================
+ Hits           5704     5779      +75     
- Misses        25780    25945     +165     
- Partials        919      926       +7
Impacted Files Coverage Δ Complexity Δ
.../java/com/owncloud/android/utils/DisplayUtils.java 23.62% <0%> (-1.27%) 0% <0%> (ø)
.../third_parties/daveKoeller/AlphanumComparator.java 80.95% <0%> (-1.2%) 0% <0%> (ø)
...om/nextcloud/client/network/ClientFactoryImpl.java 36.36% <0%> (-1.14%) 0% <0%> (ø)
...xtcloud/client/account/UserAccountManagerImpl.java 48.92% <0%> (-1.08%) 0% <0%> (ø)
...ncloud/android/ui/adapter/SyncedFolderAdapter.java 13.14% <0%> (-1%) 0% <0%> (ø)
...tcloud/client/network/ConnectivityServiceImpl.java 58.33% <0%> (-0.99%) 0% <0%> (ø)
...com/owncloud/android/ui/activity/BaseActivity.java 30.33% <0%> (-0.62%) 0% <0%> (ø)
...owncloud/android/ui/activity/SettingsActivity.java 38.46% <0%> (-0.25%) 0% <0%> (ø)
...wncloud/android/providers/FileContentProvider.java 21.6% <0%> (-0.24%) 0% <0%> (ø)
...ncloud/android/datamodel/SyncedFolderProvider.java 11.66% <0%> (-0.2%) 0% <0%> (ø)
... and 45 more

@AndyScherzinger
Copy link
Member

@tobiasKaminsky @jancborchardt see screenshot, fine from my pov, please merge after final review :)
device-2019-11-28-181634

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 good! :)

@tobiasKaminsky tobiasKaminsky merged commit d270752 into nextcloud:master Dec 2, 2019
@tobiasKaminsky
Copy link
Member

tobiasKaminsky commented Dec 2, 2019

Thanks for your PR @Shagequi 🎉

@Kilian-Perisset Kilian-Perisset deleted the feature/fix-restore-btn-size branch December 2, 2019 11:31
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.10.0 milestone Dec 2, 2019
tobiasKaminsky added a commit that referenced this pull request Dec 3, 2019
d270752 Merge pull request #4865 from Infomaniak/feature/fix-restore-btn-size
34db134 Merge pull request #4910 from nextcloud/dependabot/gradle/io.gitlab.arturbosch.detekt-detekt-gradle-plugin-1.2.1
eac9c9d Merge pull request #4911 from nextcloud/dependabot/gradle/org.mockito-mockito-core-3.2.0
dc0181b [tx-robot] updated from transifex
82e226f Bump mockito-core from 3.1.0 to 3.2.0
85e13f2 Bump detekt-gradle-plugin from 1.2.0 to 1.2.1
30f96aa daily dev 20191129
@jancborchardt
Copy link
Member

Good stuff @Shagequi! :) As mentioned above, could you also open a pull request to change the same in the server:
https://github.com/nextcloud/server/blob/master/core/img/actions/history.svg
(There’s also a .png version of it.)

Just make sure it’s the same dimensions, and that it’s compressed. You can compress SVG with the scour command in this script: https://github.com/nextcloud/server/blob/master/build/image-optimization.sh#L10

Kilian-Perisset pushed a commit to nextcloud/server that referenced this pull request Dec 3, 2019
Asked here : nextcloud/android#4865
The purpose is to replace the "too small" restore icon on server and Android.
rolandinus pushed a commit to rolandinus/server that referenced this pull request Dec 4, 2019
Asked here : nextcloud/android#4865
The purpose is to replace the "too small" restore icon on server and Android.
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.

None yet

5 participants