-
Notifications
You must be signed in to change notification settings - Fork 58
Remove obsolete/faulty css for the top right button #196
Conversation
Codecov Report
@@ Coverage Diff @@
## master #196 +/- ##
============================================
- Coverage 82.39% 81.93% -0.47%
Complexity 334 334
============================================
Files 38 38
Lines 1284 1284
============================================
- Hits 1058 1052 -6
- Misses 226 232 +6
Continue to review full report at Codecov.
|
Could you use a span and apply the icon through css instead? I would like to unify the alignment of icons inside buttons in core :) |
@skjnldsv Feel free to hijack this... |
@eppfel sorry, here you go :) |
08391e0
to
f51367e
Compare
js/gallerybutton.js
Outdated
'<img class="svg" src="' + OC.imagePath('core', 'actions/toggle-pictures.svg') + | ||
'"' + | ||
'alt="' + t('gallery', 'Gallery view') + '"/>' + | ||
'<span class="icon-toggle-pictures"></span>' + |
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.
Is there a way to keep the alternative text for the icon?
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.
With title?
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.
It currently says "Gallery view", letting people know what the button is for. It's also an accessibility thing if I'm not mistaken.
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.
Alt is in case of the image isn't loaded indeed. With title we should be able to have a popup. That's how we do on the other apps on nextcloud I think. There isn't any icons used as direct image (not that I'm aware of at least) :)
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.
Just tested on Files and seems all of these are gone anyway, so if @jancborchardt is OK with this, it can be merged.
Can you post a before/after screenshot? Not sure what the issue is. It is intentional that the switch to Gallery in the top right of files does not have a border. |
@eppfel What is the status of this? Could you show what is broken and what does this fix to evaluate if we need to ship this with 12. I would like to move this to 13. |
f51367e
to
e0fafce
Compare
Wait.. now the icon greyvalue is off... @skjnldsv to the rescue! |
Signed-off-by: Felix A. Epp <[email protected]>
Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
e0fafce
to
523d282
Compare
Rebased |
BTW: I'm currently fixing all of this here 😉 |
Signed-off-by: Morris Jobke <[email protected]>
Okay I adjusted:
|
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.
👍
And this should be backported to the stable12 branch. |
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.
Looks good! What about the Travis failure?
Someone needs to adjust the acceptance test to work with the new menu introduced in 12. |
Fixes wrong styling in files app and gallery due to new button styles in server nextcloud/server#3187