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.

Sign up for GitHub

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

Jump to bottom

Permanently visible left sidebar #3442

Merged
merged 1 commit into from
May 15, 2020
Merged

Permanently visible left sidebar #3442

merged 1 commit into from
May 15, 2020

Conversation

LukasHirt
Copy link
Contributor

@LukasHirt LukasHirt commented May 10, 2020 •
edited
Loading

Description

Made left sidebar permanently visible and move branding into it. Requires owncloud/owncloud-design-system#735

Related Issue

Screenshots (if appropriate):

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Open tasks:

  • Update ODS when released

@LukasHirt LukasHirt self-assigned this May 10, 2020
@LukasHirt LukasHirt requested review from kulmann and PVince81 and removed request for kulmann May 10, 2020 16:31
@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9651/

20200510-165822-046.png
20200510-165822-331.png
20200510-165851-017.png
20200510-165851-283.png
20200510-170600-027.png
20200510-170626-814.png

@LukasHirt
Copy link
Contributor Author

Thinking if it makes sense to implement some acceptance tests. The component itself has unit tests in the ODS so not sure what we would want to test here. Maybe just the resizing but that doesn't seem big enough for acceptance tests.

PVince81
PVince81 reviewed May 11, 2020
View reviewed changes
src/Phoenix.vue

mounted() {
this.$nextTick(() => {
window.addEventListener('resize', this.onResize)
Copy link
Contributor

Choose a reason for hiding this comment

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

note: we have a "resize-observer" plugin, but it's more designed for specific elements in the DOM, not the window. I guess you did not need it

Copy link
Contributor Author

@LukasHirt LukasHirt May 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I didn't see a reason to use it here since we are watching the whole window. Also, to try not to have an additional element in the DOM.

@PVince81
Copy link
Contributor

if the acceptance tests cover the "click to open" in responsive mode, I guess it's enough.
it likely already covers that for the parts where we switch sections in the files app

@PVince81
Copy link
Contributor

PVince81 commented May 11, 2020 •
edited
Loading

Three comments:

  1. Misplaced cross icon for closing:

image

  1. I'm kind of missing some animation when opening the sidebar, even a fast one.

  2. Missing swipe gesture on mobile for opening the sidebar, might need extra plugins ?

@PVince81
Copy link
Contributor

regarding 1 I wonder if the sidebar shouldn't just look the same and have the menu items on the bottom of the logo. if there is enough space, it doesn't need to occupy the whole screen and could take up part of it, maybe 90% or so ? not sure how mobile sidebars usually look in that case

@PVince81
Copy link
Contributor

ah, just noticed that on my mobile phone the menu items are indeed below the logo, so it's just in some bigger resolution that they move to the right. not sure if intended

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9657/

20200511-094647-517.png
20200511-094647-829.png
20200511-094717-847.png
20200511-094718-148.png
20200511-095419-876.png
20200511-095506-959.png

@LukasHirt
Copy link
Contributor Author

if the acceptance tests cover the "click to open" in responsive mode, I guess it's enough.
it likely already covers that for the parts where we switch sections in the files app

I agree. I added just a check in the tests if the sidebar is visible to know if the menu button should be clicked or not.

@LukasHirt
Copy link
Contributor Author

After bringing back oc-app-bar class because of tests is needed also this PR owncloud/owncloud-design-system#750 not to enforce the muted background.

@LukasHirt
Copy link
Contributor Author

To address comments:

  1. This was a bug. Will be fixed in the ODS PR.
  2. Will be added in the ODS PR as well.
  3. Yep, would be better to bring some library. Will push it in a separate commit to make it easier to review

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9663/

20200511-111838-079.png
20200511-111838-391.png
20200511-111927-572.png
20200511-111927-803.png
20200511-112730-188.png
20200511-112755-572.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9665/

20200511-120408-736.png
20200511-120409-106.png
20200511-120446-761.png
20200511-120447-032.png
20200511-121429-856.png
20200511-121519-165.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9667/

20200511-183002-489.png
20200511-183419-162.png
20200511-183504-780.png

@LukasHirt
Copy link
Contributor Author

@PVince81 ODS updated, swipe and transition added and responsive layout fixed. Pls re-review. 🙂

Had to update the ODS here because the new sidebar would break tests otherwise.

@kulmann
Copy link
Member

kulmann commented May 13, 2020 •
edited
Loading

Found one tiny issue: with visible sidebar the file opening banner at the bottom of the screen doesn't adjust its width. As a result, the X for closing gets pushed out of the viewport (to the right), see screenshots.

Screenshot 2020-05-13 at 12 49 05
Screenshot 2020-05-13 at 12 49 14

@LukasHirt
Copy link
Contributor Author

@kulmann Nice catch! 👍 For now I have adjusted the width directly in the component since the bottom actions off-canvas won't probably be there for long anyway.

@LukasHirt LukasHirt mentioned this pull request May 13, 2020
Merged
10 tasks
@LukasHirt LukasHirt marked this pull request as ready for review May 13, 2020 12:07
@LukasHirt LukasHirt mentioned this pull request May 13, 2020
Merged
5 tasks
kulmann
kulmann approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

🚀

PVince81
PVince81 approved these changes May 15, 2020
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

A big step forward, looks nice 👍

Note: the swipe gesture doesn't seem to work on my Android phone, let's investigate separately

@LukasHirt LukasHirt mentioned this pull request May 15, 2020
Closed
@LukasHirt LukasHirt merged commit 915778a into master May 15, 2020
@LukasHirt LukasHirt deleted the feature/branded-sidebar branch May 15, 2020 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 approved these changes

@kulmann kulmann kulmann approved these changes

Assignees

@LukasHirt LukasHirt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Always visible left (navigation) sidebar
4 participants
@LukasHirt @ownclouders @PVince81 @kulmann