make collabora icons inverted #1765
Conversation
luka-nextcloud
requested review from
jancborchardt,
julien-nc and
juliusknorr
|
@luka-nextcloud whenever it’s a pull request which changes something visual, mind adding screenshots for quicker design review? Thank you! :) |
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
Code seems fine. I'll make a real life test when I'll have deployed Collabora locally 😁.
If we decide to include the icon-collabora in a css file in this app, then the change could be included in this PR.
To keep track of what happened: At the moment, icon-collabora class is defined in this PR: nextcloud/server#28877
Signed-off-by: Luka Trovic <[email protected]>
luka-nextcloud
force-pushed
the
icon-color-invert-issue-in-bright-theme
branch
from
7a82fc9 to
8929a8c
Compare
|
@eneiluj All changes are now here :) @jancborchardt You can find out screenshots: |
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
Code looks good. You could put the class in a variable for readability but that's fine like that too.
css/viewer.scss
Outdated
| } | ||
|
|
||
| .icon-collabora { | ||
| cursor: pointer; |
There was a problem hiding this comment.
This should already be done with https://github.com/nextcloud/server/blob/ee987d74303cb38b864f96660cd2ee6d6552ebfd/core/css/styles.scss#L62
@jancborchardt Right?
@luka-nextcloud Is it not?
jancborchardt
left a comment
jancborchardt
left a comment
There was a problem hiding this comment.
Good design-wise, thank you!
| .icon-collabora { | ||
| cursor: pointer; | ||
| opacity: 0.6; | ||
| &:hover { | ||
| opacity: 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Should use tabs instead of spaces 😉
src/view/FilesAppIntegration.js
Outdated
| _addHeaderShareButton() { | ||
| if ($('header').length) { | ||
| const $button = $('<div id="richdocuments-sharing"><a class="icon-shared icon-white"></a></div>') | ||
| const isInverted = window.OCA.Theming.inverted |
There was a problem hiding this comment.
To be on the safe side if the theming app is disabled:
| const isInverted = window.OCA.Theming.inverted | |
| const isInverted = Boolean(window?.OCA?.Theming?.inverted) |
src/view/FilesAppIntegration.js
Outdated
| OC.unregisterMenu($('#richdocuments-actions .icon-more'), $('#richdocuments-actions-menu')) | ||
| $('#richdocuments-actions').remove() | ||
| const actionsContainer = $('<div id="richdocuments-actions"><div class="icon-more icon-white"></div><ul id="richdocuments-actions-menu" class="popovermenu"></ul></div>') | ||
| const isInverted = window.OCA.Theming.inverted |
There was a problem hiding this comment.
juliusknorr
left a comment
juliusknorr
left a comment
There was a problem hiding this comment.
Small code style comments, otherwise looks good 👍
Signed-off-by: Luka Trovic <[email protected]>
|
@luka-nextcloud Quick tip for the pull request description, if you mention the issue number in the description of your pull request it is easier to check the original report and the issue will also be closed automatically once the PR gets merged, e.g. with |
Signed-off-by: Luka Trovic [email protected]
Summary
The color of the
shareandmorebuttons that appear on the header for office documents is now inverted according to the change of the theme color.http://prntscr.com/1sld74l