Allow to toggle sidebar and always show the toggle #989

marcoambrosini · 2020-04-03T17:41:38Z

Move the AppNavigationToggle in the AppNavigation
Move from #app-navigation IDs to have independent styling from server
Move AppNavigation open state IN the AppNavigation
Create a global toggle-navigation to emit with th event bus for any app to open the navigation (was originally the start of this pair programming, we ended up fixing far too much ^^')
Allow to toggle the navigation even when in desktop mode
Changed the animation/css from translate and shifting the AppContent to the left to using proper flex layout and not weird absolute positioning anymore 🎉
In mobile, opening the navigation does NOT shift the content, it just hovers it now. It avoid changing the whole layout and improve the rendering performances
Document the new global event (cc @ChristophWurst)
Emit event at first load

QA

@ma12-co, @skjnldsv & @ChristophWurst on a nice beer programming session on a sweet Friday afternoon 🍻

skjnldsv · 2020-04-03T18:01:39Z

korelstar

Nice work! I've tested the current implementation and found out that scrolling the app-navigation is not possible anymore. This is required for small screens!

jancborchardt · 2020-04-06T09:36:11Z

Very nice!

optional remember state ?

What is meant by this? If we have remembering, it should be per-app, and it should always work, not giving app developers a way to disable it.

skjnldsv · 2020-04-06T09:38:06Z

What is meant by this? If we have remembering, it should be per-app, and it should always work, not giving app developers a way to disable it.

It means it can be in a separate pr to keep it clean. 😉

jancborchardt

Just to make sure it’s not merged before:
We should make sure that the spec for "New message" and "New contact" buttons as discussed in https://github.com/nextcloud/server/issues/16934#issuecomment-542201187 is implemented before merging this. Otherwise the issue of "Mobile menu toggle for navigation overlaps with content (especially in Mail & Contacts)" will not only exist on mobile but on desktop too.

georgehrke · 2020-04-06T12:39:34Z

@ma12-co Thanks! This also fixes an issue we had in the calendar, where the toggle was shown, even though there is no app-navigation. ❤️

marcoambrosini · 2020-04-06T12:50:03Z

you should thank @skjnldsv more than me :)

skjnldsv · 2020-04-07T10:54:15Z

We should make sure that the spec for "New message" and "New contact" buttons as discussed in nextcloud/server#16934 (comment) is implemented before merging this. Otherwise the issue of "Mobile menu toggle for navigation overlaps with content (especially in Mail & Contacts)" will not only exist on mobile but on desktop too.

I don't know actually.
I would rather keep it simple here, otherwise it's harder to review.
Also because technically they're not the same and require us to call and decide of the proper implementation.
This doesn't create regressions technically. Let's fix the remaining issues in OP and merge it :)

korelstar

Nice work! I've tested the current implementation and found out that scrolling the app-navigation is not possible anymore. This is required for small screens!

In addition to this: the settings are not shown at the bottom anymore. Maybe, this issue is related to the above.

src/components/AppContent/AppContent.vue

marcoambrosini · 2020-04-08T11:52:58Z

@skjnldsv I guess those conflicts are because of #1002, what should I do here?

skjnldsv · 2020-04-08T12:17:07Z

@skjnldsv I guess those conflicts are because of #1002, what should I do here?

Git rebase with strategy ours to keep the current master files in case of conflict and manually npm i @nextcloud/event-bus again

korelstar

The two issues I've found are fixed, now. Looks very good, thanks!

skjnldsv · 2020-04-08T18:50:17Z

Docs fails https://app.netlify.com/sites/nextcloud-vue-components/deploys/5e8e1c5c0203164eaddaf323

https://deploy-preview-989--nextcloud-vue-components.netlify.com/

skjnldsv · 2020-04-08T19:19:42Z

WORKED

marcoambrosini · 2020-04-08T19:21:31Z

WORKED

Oh nice!

Signed-off-by: Marco Ambrosini <[email protected]>

skjnldsv

Rebased & ready to go

jancborchardt · 2020-04-08T20:00:26Z

We should make sure that the spec for "New message" and "New contact" buttons as discussed in nextcloud/server#16934 (comment) is implemented before merging this. Otherwise the issue of "Mobile menu toggle for navigation overlaps with content (especially in Mail & Contacts)" will not only exist on mobile but on desktop too.

I don't know actually.
I would rather keep it simple here, otherwise it's harder to review.
Also because technically they're not the same and require us to call and decide of the proper implementation.
This doesn't create regressions technically. Let's fix the remaining issues in OP and merge it :)

Fine with me as well, as then it will make it obvious that it will have to be fixed. :D

marcoambrosini added the 3. to review Waiting for reviews label Apr 3, 2020

marcoambrosini requested review from ChristophWurst, korelstar and skjnldsv April 3, 2020 17:41

marcoambrosini self-assigned this Apr 3, 2020

marcoambrosini requested a review from juliusknorr April 3, 2020 17:42

skjnldsv added feature: app-content Related to the app-content component feature: app-navigation Related to the app-navigation component labels Apr 3, 2020

marcoambrosini mentioned this pull request Apr 3, 2020

Add search shortcut support nextcloud/spreed#3257

Merged

1 task

marcoambrosini force-pushed the feature/noid/improve-layout branch from 150cf99 to 5914574 Compare April 3, 2020 17:47

skjnldsv requested a review from jancborchardt April 3, 2020 17:50

skjnldsv changed the title ~~Feature/noid/improve layout~~ Allow to toggle sidebar and always show the toggle Apr 3, 2020

skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 3, 2020

korelstar reviewed Apr 3, 2020

View reviewed changes

skjnldsv added the enhancement New feature or request label Apr 3, 2020

This comment has been minimized.

Sign in to view

skjnldsv mentioned this pull request Apr 3, 2020

Open and close AppNavigation #791

Closed

3 tasks

jancborchardt requested changes Apr 6, 2020

View reviewed changes

skjnldsv mentioned this pull request Apr 7, 2020

Datepicker in sidebar cut-off at certain zoom levels #998

Closed

korelstar reviewed Apr 8, 2020

View reviewed changes

juliusknorr reviewed Apr 8, 2020

View reviewed changes

src/components/AppContent/AppContent.vue Outdated Show resolved Hide resolved

marcoambrosini requested a review from jancborchardt April 8, 2020 11:08

ChristophWurst reviewed Apr 8, 2020

View reviewed changes

src/components/AppContent/AppContent.vue Outdated Show resolved Hide resolved

marcoambrosini requested a review from ChristophWurst April 8, 2020 11:15

korelstar approved these changes Apr 8, 2020

View reviewed changes

marcoambrosini force-pushed the feature/noid/improve-layout branch from 082e251 to d777c0a Compare April 8, 2020 15:16

skjnldsv force-pushed the feature/noid/improve-layout branch from d777c0a to a106a2c Compare April 8, 2020 19:02

skjnldsv force-pushed the feature/noid/improve-layout branch 2 times, most recently from e194cf6 to 520c32f Compare April 8, 2020 19:28

skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 8, 2020

Allow to toggle sidebar and always show the toggle

6c6bd17

Signed-off-by: Marco Ambrosini <[email protected]>

skjnldsv force-pushed the feature/noid/improve-layout branch from 520c32f to 6c6bd17 Compare April 8, 2020 19:31

skjnldsv approved these changes Apr 8, 2020

View reviewed changes

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 8, 2020

skjnldsv merged commit 89fab6e into master Apr 8, 2020

skjnldsv deleted the feature/noid/improve-layout branch April 8, 2020 19:36

raimund-schluessler mentioned this pull request Apr 15, 2020

Adjust breadcrumbs on resize of app-content #1021

Closed

raimund-schluessler mentioned this pull request Apr 30, 2020

Adjust Tasks to new nextcloud/vue version nextcloud/tasks#965

Merged

georgehrke mentioned this pull request May 19, 2020

Resizing window result in additionnal blank sidebar nextcloud/calendar#2276

Closed

raimund-schluessler mentioned this pull request May 23, 2020

AppNavigationSettings aren't visible on chromium mobile #1105

Closed

juliusknorr mentioned this pull request Jun 2, 2020

Changes in v2.0.0? #1129

Closed

ShGKme mentioned this pull request Nov 22, 2023

*Close sidebar* focused and visible after page load on mobile nextcloud/spreed#10908

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to toggle sidebar and always show the toggle #989

Allow to toggle sidebar and always show the toggle #989

marcoambrosini commented Apr 3, 2020 •

edited

Loading

skjnldsv commented Apr 3, 2020

korelstar left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jancborchardt commented Apr 6, 2020

skjnldsv commented Apr 6, 2020 •

edited

Loading

jancborchardt left a comment

georgehrke commented Apr 6, 2020

marcoambrosini commented Apr 6, 2020

skjnldsv commented Apr 7, 2020

korelstar left a comment

marcoambrosini commented Apr 8, 2020

skjnldsv commented Apr 8, 2020 •

edited

Loading

korelstar left a comment

skjnldsv commented Apr 8, 2020 •

edited

Loading

skjnldsv commented Apr 8, 2020

marcoambrosini commented Apr 8, 2020 •

edited

Loading

skjnldsv left a comment

jancborchardt commented Apr 8, 2020

Allow to toggle sidebar and always show the toggle #989

Allow to toggle sidebar and always show the toggle #989

Conversation

marcoambrosini commented Apr 3, 2020 • edited Loading

QA

skjnldsv commented Apr 3, 2020

korelstar left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jancborchardt commented Apr 6, 2020

skjnldsv commented Apr 6, 2020 • edited Loading

jancborchardt left a comment

Choose a reason for hiding this comment

georgehrke commented Apr 6, 2020

marcoambrosini commented Apr 6, 2020

skjnldsv commented Apr 7, 2020

korelstar left a comment

Choose a reason for hiding this comment

marcoambrosini commented Apr 8, 2020

skjnldsv commented Apr 8, 2020 • edited Loading

korelstar left a comment

Choose a reason for hiding this comment

skjnldsv commented Apr 8, 2020 • edited Loading

skjnldsv commented Apr 8, 2020

marcoambrosini commented Apr 8, 2020 • edited Loading

skjnldsv left a comment

Choose a reason for hiding this comment

jancborchardt commented Apr 8, 2020

marcoambrosini commented Apr 3, 2020 •

edited

Loading

skjnldsv commented Apr 6, 2020 •

edited

Loading

skjnldsv commented Apr 8, 2020 •

edited

Loading

skjnldsv commented Apr 8, 2020 •

edited

Loading

marcoambrosini commented Apr 8, 2020 •

edited

Loading