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

Mobile Navigation #390

Merged
merged 45 commits into from
Aug 27, 2020
Merged

Mobile Navigation #390

merged 45 commits into from
Aug 27, 2020

Conversation

theovier
Copy link
Collaborator

@theovier theovier commented Aug 21, 2020

Description

Implements issue #284

Reason for this PR

  • Users want to smoothly navigate on our website even on mobile devices.

Changes in this PR

  • Add mobile navigation bar on smaller screen sizes.

Type of change

  • New feature

How Has This Been Tested?

  • Manually tested all use cases this could alter

Test Configuration:

  • OS: Windows Mac Linux

  • Browser: Firefox Chrome Safari Chromium

  • Frontend:

    • Development build
  • Backend:

    • No backend needed to test this

Checklist:

  • I added corresponding E2E tests
  • I have performed a self-review of my own code
  • My changes generate no new linting warnings or console warnings
  • I have updated the CHANGELOG.md to include any changes made in this PR

@theovier theovier added vue Epic responsive visual Something visual in the user interface labels Aug 21, 2020
@theovier theovier requested a review from bastihav August 21, 2020 09:18
@theovier theovier self-assigned this Aug 21, 2020
@theovier
Copy link
Collaborator Author

WIP: Missing IDs to target mobile navbar elements in cypress tests.

Copy link
Contributor

@tobias-wiese tobias-wiese left a comment

Choose a reason for hiding this comment

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

looks fancy.
I would suggest 2 things:

  1. If one navigates via the burger menu, I'd expect the burger menu to close when the target route is reached.
  2. If a user opens the burger menu for the first time, the submenus (e.g. My Profile) are expanded by default. They should be collapsed at this point to provide a better overview.

@tobias-wiese tobias-wiese self-requested a review August 26, 2020 09:53
Copy link
Contributor

@tobias-wiese tobias-wiese left a comment

Choose a reason for hiding this comment

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

The menu points are not aligned properly

grafik

@theovier
Copy link
Collaborator Author

theovier commented Aug 26, 2020

The menu points are not aligned properly

grafik

Can you provide context? When does this happen?

@theovier
Copy link
Collaborator Author

looks fancy.
I would suggest 2 things:

  1. If one navigates via the burger menu, I'd expect the burger menu to close when the target route is reached.
  2. If a user opens the burger menu for the first time, the submenus (e.g. My Profile) are expanded by default. They should be collapsed at this point to provide a better overview.

Acknowledged.

  1. I am unsure what the normal procedure would be here since I might end up on the wrong subsite and want to navigate fast to another section which I actually searched for. However, if @bastihav feels the same, we can collapse the burger menu on route change.
  2. This was done purposely to show all the options a user has since we do not have really many submenus altogether. Again, this might be preference so I don't have a problem changing it.

@tobias-wiese
Copy link
Contributor

The menu points are not aligned properly
grafik

Can you provide context? When does this happen?

This seem to happen in firefox only, at any screensize where the burger menu is shown

@theovier
Copy link
Collaborator Author

theovier commented Aug 26, 2020

The menu points are not aligned properly
grafik

Can you provide context? When does this happen?

This seem to happen in firefox only, at any screensize where the burger menu is shown

I will look into this.

Fixed by f49541c

@bastihav
Copy link
Collaborator

looks fancy.
I would suggest 2 things:

  1. If one navigates via the burger menu, I'd expect the burger menu to close when the target route is reached.
  2. If a user opens the burger menu for the first time, the submenus (e.g. My Profile) are expanded by default. They should be collapsed at this point to provide a better overview.

Acknowledged.

  1. I am unsure what the normal procedure would be here since I might end up on the wrong subsite and want to navigate fast to another section which I actually searched for. However, if @bastihav feels the same, we can collapse the burger menu on route change.

I think closing the menu is the normal procedure, I just checked a couple of websites, none of which kept the menu open.

  1. This was done purposely to show all the options a user has since we do not have really many submenus altogether. Again, this might be preference so I don't have a problem changing it.

I agree that we do not have many submenus at this moment, but if we are showing all options by default, we would not need sub menus at all. The point of sub menus is, that you organize menu points and "make the UI a bit more organized", so I would agree with Tobi here.

@theovier
Copy link
Collaborator Author

theovier commented Aug 26, 2020

looks fancy.
I would suggest 2 things:

  1. If one navigates via the burger menu, I'd expect the burger menu to close when the target route is reached.
  2. If a user opens the burger menu for the first time, the submenus (e.g. My Profile) are expanded by default. They should be collapsed at this point to provide a better overview.
  1. seems to pose some problems as I was unable to use the route guards in the mobile base navbar component. Will have to look further into this as it seems not to be as easy as assumed.

  2. is implemented by af20774.

@tobias-wiese
Copy link
Contributor

looks fancy.
I would suggest 2 things:

  1. If one navigates via the burger menu, I'd expect the burger menu to close when the target route is reached.
  2. If a user opens the burger menu for the first time, the submenus (e.g. My Profile) are expanded by default. They should be collapsed at this point to provide a better overview.
1. seems to pose some problems as I was unable to use the route guards in the mobile base navbar component. Will have to look further into this as it seems not to be as easy as assumed.

2. is implemented by [af20774](https://github.com/upb-uc4/ui-web/commit/af2077412d7a401aaca6e6595d993580d60d14e6).

To 1:
I think the problem is, that you cannot call a navigation guard out of a component, that is not the routed component (declared in router). I had this problem, too. As the navbar is a component, that is not part of the routing structure, I do not believe that we can use navigation guards for this purpose. We may implement a function

function navigate(target: string) {
    isOpen.value = false;
    Router.push(target);
}

that is then called by each navbar item. Not pretty, but it should do the job.

@theovier
Copy link
Collaborator Author

looks fancy.
I would suggest 2 things:

  1. If one navigates via the burger menu, I'd expect the burger menu to close when the target route is reached.
  2. If a user opens the burger menu for the first time, the submenus (e.g. My Profile) are expanded by default. They should be collapsed at this point to provide a better overview.
1. seems to pose some problems as I was unable to use the route guards in the mobile base navbar component. Will have to look further into this as it seems not to be as easy as assumed.

2. is implemented by [af20774](https://github.com/upb-uc4/ui-web/commit/af2077412d7a401aaca6e6595d993580d60d14e6).

To 1:
I think the problem is, that you cannot call a navigation guard out of a component, that is not the routed component (declared in router). I had this problem, too. As the navbar is a component, that is not part of the routing structure, I do not believe that we can use navigation guards for this purpose. We may implement a function

function navigate(target: string) {
    isOpen.value = false;
    Router.push(target);
}

that is then called by each navbar item. Not pretty, but it should do the job.

You are correct, that was the issue I also assumed. However, calling this in the menuItems is not a solution as they are children of children of children of the navbar. We would have to emit several levels in order to achieve this. Might look into using the store for this. However, I don't like any of the solutions to be honest.

@tobias-wiese
Copy link
Contributor

looks fancy.
I would suggest 2 things:

  1. If one navigates via the burger menu, I'd expect the burger menu to close when the target route is reached.
  2. If a user opens the burger menu for the first time, the submenus (e.g. My Profile) are expanded by default. They should be collapsed at this point to provide a better overview.
1. seems to pose some problems as I was unable to use the route guards in the mobile base navbar component. Will have to look further into this as it seems not to be as easy as assumed.

2. is implemented by [af20774](https://github.com/upb-uc4/ui-web/commit/af2077412d7a401aaca6e6595d993580d60d14e6).

To 1:
I think the problem is, that you cannot call a navigation guard out of a component, that is not the routed component (declared in router). I had this problem, too. As the navbar is a component, that is not part of the routing structure, I do not believe that we can use navigation guards for this purpose. We may implement a function

function navigate(target: string) {
    isOpen.value = false;
    Router.push(target);
}

that is then called by each navbar item. Not pretty, but it should do the job.

You are correct, that was the issue I also assumed. However, calling this in the menuItems is not a solution as they are children of children of children of the navbar. We would have to emit several levels in order to achieve this. Might look into using the store for this. However, I don't like any of the solutions to be honest.

We could add an issue for 1 and merge this feature if we do not find a solution until tomorrow, as it is technically complete.

Copy link
Contributor

@tobias-wiese tobias-wiese left a comment

Choose a reason for hiding this comment

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

As the wrong margins are corrected, I will approve this. If you want, you can merge the branch without fixing the collapsing of the burger menu, but we should catch up this asap then :)

@theovier theovier merged commit 53d4eb5 into develop Aug 27, 2020
@theovier theovier deleted the feature/mobile_navbar branch August 27, 2020 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Epic responsive visual Something visual in the user interface vue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants