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

Always show sidebar toggle, also when sidebar is open #587

Merged
merged 3 commits into from
Apr 20, 2018

Conversation

jancborchardt
Copy link
Member

This makes the interface more predictable and doesn’t leave the fullscreen toggle hanging in mid-air in some settings.

@Ivansss @danxuliu we only need to add the handler to make the sidebar-trigger close the sidebar again. I guess it’s in sidebarview.js but don’t know fully how, can you help me here? :)

@jancborchardt jancborchardt added this to the 3.1 (Nextcloud 13.0.2/3) milestone Jan 12, 2018
@danxuliu
Copy link
Member

Clicking on the sidebar toggle when the sidebar is closed opens it. But clicking on it when it is open does nothing. That is a bit strange.

However, clicking on the sidebar toggle to close it... also feels a bit strange for me :-P Probably it is just because I am already used to it (also from the Files app), but the most natural way to close the sidebar is using the Close button. However, if the sidebar toggle closes the sidebar, should the Close button be removed? Or could it be kept?

In any case, if the sidebar toggle toggles the sidebar then its appearance should reflect this and it should have a higher opacity or something like that when the sidebar is open.

@nickvergessen nickvergessen changed the base branch from master to stable13 January 15, 2018 13:45
@jancborchardt
Copy link
Member Author

@danxuliu that’s why I wrote:

we only need to add the handler to make the sidebar-trigger close the sidebar again. I guess it’s in sidebarview.js but don’t know fully how, can you help me here? :)

;)

Probably it is just because I am already used to it (also from the Files app), but the most natural way to close the sidebar is using the Close button.

There’s no natural way. With the left navigation, we have the the menu toggle as well. Apart from clicking the menu toggle again, clicking on the empty part below where the content is works again, to go back to the content. Or of course when you click a navigation entry which then gets loaded in the content.

However, if the sidebar toggle closes the sidebar, should the Close button be removed? Or could it be kept?

When we fix the width of the right sidebar as per #569 (instead of filling whole space on mobile) so the button is visible when the sidebar is open, we can do the same here as for the left navigation. And yes, probably remove the close button. Which I would like a lot since it interferes with content elements like the call name, name editing, and copy link functions.

@danxuliu
Copy link
Member

@jancborchardt

that’s why I wrote:

we only need to add the handler to make the sidebar-trigger close the sidebar again. I guess it’s in sidebarview.js but don’t know fully how, can you help me here? :)

🤦 Sorry, my bad ;-)

When we fix the width of the right sidebar as per #569

To be honest I hope that it will not be merged :-P But I do not understand, how is that related to this pull request? As long as the sidebar toggle is always shown next to the sidebar, why does it matter whether the size is fixed or not?

And yes, probably remove the close button. Which I would like a lot since it interferes with content elements like the call name, name editing, and copy link functions.

OK, no problem; I can live without Close button :-P

@danxuliu
Copy link
Member

Just for your information, I have been working on the missing part, but I found a related issue in server that is taking me longer than expected to fix :-(

@jancborchardt
Copy link
Member Author

To be honest I hope that it will not be merged :-P But I do not understand, how is that related to this pull request? As long as the sidebar toggle is always shown next to the sidebar, why does it matter whether the size is fixed or not?

The problem is that on mobile, the sidebar will take up 100% of width. Hence we can not remove the x Close button, since otherwise you can not exit the sidebar again because the sidebar toggle left outside of the sidebar will not be shown.

@nickvergessen nickvergessen modified the milestones: 3.2 (Nextcloud 13.0.2), 4.0 (Nextcloud 14) Mar 9, 2018
@nickvergessen nickvergessen changed the base branch from stable13 to master March 9, 2018 15:00
@nickvergessen
Copy link
Member

will take over

@nickvergessen nickvergessen deleted the sidebar-toggle branch March 28, 2018 11:37
@nickvergessen nickvergessen restored the sidebar-toggle branch March 28, 2018 13:38
@nickvergessen nickvergessen reopened this Mar 28, 2018
@nickvergessen
Copy link
Member

Can't be handled the same way as the fullscreen icon, so reopening

@@ -139,7 +139,7 @@
return '<span><div class="avatar" data-user="' + escapeHTML(element.id) + '" data-user-display-name="' + escapeHTML(element.displayName) + '"></div>' + escapeHTML(element.displayName) + '</span>';
},
formatSelection: function () {
return '<span class="select2-default" style="padding-left: 0;">'+OC.L10N.translate('spreed', 'Choose person…')+'</span>';
return '<span class="select2-default" style="padding-left: 0;">'+OC.L10N.translate('spreed', 'New call …')+'</span>';
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong, it adds a user to the participant list.
Anyway the naming is fixed in #751

Should not be done in a PR that deals with "sidebar toggle"

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@Ivansss
Copy link
Member

Ivansss commented Apr 5, 2018

Rebased to master

@Ivansss
Copy link
Member

Ivansss commented Apr 5, 2018

Didn't remove the close button because when app is in 'fullscreen' the sidebar overlays chat view and ('fullscreen' & 'sidebar-toggle') buttons.
This should be fixed in another PR.

Signed-off-by: Ivan Sein <ivan@nextcloud.com>
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

The position of the toggle button needs to be fixed in public pages due to nextcloud/server#9002 That will be done in a different pull request, though, as that should not be backported while the rest of this pull request should.

@nickvergessen
Copy link
Member

The position of the toggle button needs to be fixed in public pages due to nextcloud/server#9002 That will be done in a different pull request, though, as that should not be backported while the rest of this pull request should.

@danxuliu can you do this?

@danxuliu
Copy link
Member

can you do this?

Yes, I will do that.

marcoambrosini pushed a commit that referenced this pull request Oct 9, 2019
Add ActionRadio
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.

4 participants