-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix some minor settings UI issues #10028
Conversation
nice 👍 |
Codecov Report
@@ Coverage Diff @@
## master #10028 +/- ##
=========================================
Coverage 31.72% 31.72%
Complexity 26007 26007
=========================================
Files 1661 1660 -1
Lines 96132 96132
Branches 1290 1290
=========================================
Hits 30494 30494
Misses 65638 65638
|
3a12b29
to
845fe15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it and it works 👍
Unit tests fail and it needs to be merged: |
845fe15
to
6d59a82
Compare
@MorrisJobke Fixed the CI failure. The current one seems unrelated to me in acceptance-app-files https://drone.nextcloud.com/nextcloud/server/8517/230 Since the js merge is not related i moved it to #10043 |
Failure unrelated it seems :) |
6d59a82
to
ea45519
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works 👍
It seems that the acceptance test are broken then:
@danxuliu Could you help with that? |
Seems that is was caused by the reset button being hidden, but it makes sense to keep it, so you can abort the action before you create a new tag. Let's see if that works. |
Indeed; before a new tag is created the reset button was clicked to ensure that no tag was being updated (there is currently no test in which that could happen, but it was added just to be sure). As the reset button was hidden it could not be clicked and the test failed.
I was going to remove the click on the reset button instead, but you are right in that it makes sense to keep it visible 👍
It does :-) |
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Rebased for CI |
1c60fc3
to
94dcddc
Compare
@juliushaertl just a small comment on the collaborative tags UI: Seeing as there are 3 icons without label, the "Delete" and "Reset" buttons should be in a 3-dot-menu, no? |
@jancborchardt I'd say this is a special case, since the user is already in edit mode when this ui is shown. Therefore those 3 actions are equally important and we cannot determine a default action which will always be used. So having all those in a 3 dot menu would lead to one additional click for any action the user would take. |
Before:
After:
Before:
After:
Once we have some docs about the collaborative tags we should also add a link to that in the settings. nextcloud/documentation#789