Use more applicable icon for 'Open sidebar'#142
Conversation
jancborchardt
added
enhancement
| icon="icon-menu-sidebar-white-forced" | ||
| @click="showSharingSidebar"> | ||
| {{ t('viewer', 'Share') }} | ||
| {{ t('viewer', 'Open sidebar') }} |
There was a problem hiding this comment.
This is not only opening the sidebar, this is opening the sharing section of the sidebar.
Therefore if sharing is disabled, this button will be hidden. The showSharingSidebar needs to be updated accordingly alongside the v-if="OCA.Sharing" :)
There was a problem hiding this comment.
For all intents and purposes, this button is the only one in Viewer which opens the sidebar. :) So that’s what it should be called.
Therefore if sharing is disabled, this button will be hidden.
And then there’s also no way to open the sidebar from the viewer, right?
Therefore I think we should merge this and move any further updates to the menu implementation PR → #15
There was a problem hiding this comment.
No, that would leave the current state of the viewer in a not optimal one. :)
Please take the time to adjust this pr instead!
#15 is much more work
There was a problem hiding this comment.
ahh ok. of course it should also be possible to open the sidebar even if sharing is disabled.
There was a problem hiding this comment.
No, that would leave the current state of the viewer in a not optimal one. :)
I don’t understand how, because:
- If Sharing is enabled, the button is shown and it does open the sidebar.
- If Sharing is not enabled, the icon/button is not shown.
So what about that is not optimal, or less optimal than now?
If it should be changed to not rely on Sharing I would love to do that but need to know how. :) (But even then I don’t understand how that is part of this purely cosmetic change.)
There was a problem hiding this comment.
Ah, yes, you're right.
My point was that now that we tell the user that the button open the sidebar, s.he might get confused when the button disappear if sharing is disabled. Because there is still other stuff you can do in the sidebar :)
skjnldsv
force-pushed
the
design/sidebar-icon
branch
from
a5d41ab to
b66f0ae
Compare
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
skjnldsv
force-pushed
the
design/sidebar-icon
branch
from
b66f0ae to
9404837
Compare
|
As per nextcloud/server#15997 (comment) → let’s get this in. :) |
|
Tests are ok now! |
|
/backport to stable16 |
|
The backport to stable16 failed. Please do this backport manually. |
|
@skjnldsv @juliushaertl I guess it needs to be backported to be in this intermediate release we talked about? How do we best do it? |
|
We need to to the backport for stable16 so it is in the next maintenance release, but the bot failed so it needs to be done manually. |
5 participants
As discussed @juliushaertl @skjnldsv @karlitschek → this is the same icon we already use for the sidebar in the Talk app.
We should probably also backport this?