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

Separated navigation #3013

Merged
merged 1 commit into from
Feb 13, 2020
Merged

Separated navigation #3013

merged 1 commit into from
Feb 13, 2020

Conversation

LukasHirt
Copy link
Collaborator

@LukasHirt LukasHirt commented Feb 12, 2020

Description

In the sidebar is now only extension-specific navigation and into the apps switcher are automatically injected extensions which have at least one nav item. In the case that extension has only one nav item, the menu button is replaced by the name of the extension.

Related Issue

How Has This Been Tested?

  • test environment: Manually

Screenshots (if appropriate):

image

image

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

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@LukasHirt LukasHirt added Category:Enhancement Add new functionality Status:Needs-Review Needs review from a maintainer labels Feb 12, 2020
@LukasHirt LukasHirt self-assigned this Feb 12, 2020
@update-docs
Copy link

update-docs bot commented Feb 12, 2020

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@LukasHirt
Copy link
Collaborator Author

LukasHirt commented Feb 12, 2020

I have a small issue when displaying some title for the current extension. Currently, it is displayed only in case of extension having only one nav item when it replaces the menu button. But when we have the menu button there it doesn't look nice. I'm thinking if this might be solved with having the sidebar always visible as was once already discussed. Then this issue would be solved. I'm just a bit scared of how it would look like when switching between extension with menu and without. Suddenly appearing sidebar might not be so nice in the end as well.

Anyway, seems to me like a problem for a separate issue.

@LukasHirt LukasHirt force-pushed the feature/separate-nav branch from e415e91 to ac0f188 Compare February 12, 2020 10:15
@LukasHirt LukasHirt removed the Category:Enhancement Add new functionality label Feb 12, 2020
@LukasHirt LukasHirt force-pushed the feature/separate-nav branch 2 times, most recently from 50a2ad0 to 099420d Compare February 12, 2020 11:12
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.

Looks fine so far, see comments

src/Phoenix.vue Outdated Show resolved Hide resolved
src/Phoenix.vue Show resolved Hide resolved
src/Phoenix.vue Show resolved Hide resolved
@PVince81
Copy link
Contributor

I guess not easy to add acceptance tests ?

Regarding the trouble with the sidebar button: in what scenario would the button/sidebar flicker ? Is it when switching between an app that has a sidebar to one that doesn't ?

@LukasHirt
Copy link
Collaborator Author

I guess not easy to add acceptance tests ?

We'd need to load some other extension with routes to test the app switcher. And TBH I didn't want to implement acceptance tests for this part because I think this should be covered with unit tests. As soon as they're set up in Phoenix they can be implemented. I raised an issue for it here #3020

@LukasHirt
Copy link
Collaborator Author

Regarding the trouble with the sidebar button: in what scenario would the button/sidebar flicker ? Is it when switching between an app that has a sidebar to one that doesn't ?

Exactly. Basically scenario when an extension has only one nav item.

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.

Getting the currentExtensionName from an app that is only defined in applications (see config.json.sample) doesn't work. When I click on the app, the menu with burger icon stays and the js console throws an error. See comment for details.

src/Phoenix.vue Outdated Show resolved Hide resolved
src/Phoenix.vue Outdated Show resolved Hide resolved
src/components/Top-Bar.vue Show resolved Hide resolved
@LukasHirt LukasHirt force-pushed the feature/separate-nav branch from 74d7cf4 to 0c7f747 Compare February 13, 2020 09:47
…o app switcher

Added translation

Changed methods name and removed arguments

Extended toggle name
@LukasHirt LukasHirt force-pushed the feature/separate-nav branch from 0c7f747 to c775001 Compare February 13, 2020 09:51
@LukasHirt
Copy link
Collaborator Author

@kulmann Changes implemented or addressed.

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 merged commit ae6ac7d into master Feb 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the feature/separate-nav branch February 13, 2020 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Needs-Review Needs review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extension points app navigation vs app switcher
3 participants