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

Move location picker hint under breadcrumbs #5008

Merged
merged 1 commit into from
Apr 26, 2021

Conversation

LukasHirt
Copy link
Collaborator

@LukasHirt LukasHirt commented Apr 23, 2021

Description

We've moved the hint that is describing how to use the Location picker from sidebar under the breadcrumbs. There is navigation of the Files extension displayed in the sidebar now instead.

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

@LukasHirt LukasHirt added the Category:Enhancement Add new functionality label Apr 23, 2021
@LukasHirt LukasHirt self-assigned this Apr 23, 2021
Copy link
Contributor

@pascalwengerter pascalwengerter left a comment

Choose a reason for hiding this comment

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

Looks great visually, yet I sometimes end up with a forever loading spinner, an PROPFIND error or an Uncaught (in promise) error

@@ -158,7 +154,7 @@ export default {
},

sidebarNavItems() {
if (this.publicPage()) {
if (!this.user.token) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the motivation behind this change here?

Copy link
Collaborator Author

@LukasHirt LukasHirt Apr 23, 2021

Choose a reason for hiding this comment

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

publicPage is true for a page that can be used in both authorised/unauthorised context (location picker, media viewer, etc.). So if the user would be in location picker with his own private resources he wouldn't see the navigation because it is a public page.

@@ -26,14 +26,10 @@
:logo-img="logoImage"
:product-name="productName"
:nav-items="sidebarNavItems"
:hide-nav="sidebar.navigationHidden"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we removing the general capability of hidding the sidebar? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The whole sidebar can be still hidden with isSidebarVisible. This is only about the navigation items. And from my understanding the sidebar should behave now only as navigation so I do not see much point in giving the option to hide it 🤷

@LukasHirt
Copy link
Collaborator Author

I sometimes end up with a forever loading spinner, an PROPFIND error or an Uncaught (in promise) error

I guess that happens only in the location picker? I'll dig into it

@kulmann kulmann changed the base branch from master to a11y-swarming April 26, 2021 08:55
@kulmann kulmann merged commit bb9e4c1 into a11y-swarming Apr 26, 2021
@delete-merged-branch delete-merged-branch bot deleted the feat/move-location-picker-hint branch April 26, 2021 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Enhancement Add new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants