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

feat: support mobile history back button #475

Closed
wants to merge 10 commits into from

Conversation

lukeromanowicz
Copy link
Contributor

@lukeromanowicz lukeromanowicz commented Aug 24, 2023

Description

Resolves #337

I agree with @janmichek that this task ended up being quite a challenge. I managed to more or less get the expected result but it isn't perfect.

Demo

2023-08-30.12-19-26.mp4

Checklist:

  • I have read and followed the Contributing Guide
  • My change does not require a change to the documentation.

@github-actions
Copy link

Deployed to https://pr-475-aescan.stg.aepps.com

@CharisSiarampalis
Copy link

@lukeromanowicz The navigation is open when going back to the previous page. Shouldn't it be close so that the user doesn't have to scroll down?

@lukeromanowicz
Copy link
Contributor Author

This is a good question. IMO if we consider the menu to be vital part of the navigation that should be supporting navigation through history button that I would say yes - it should show up when using the history buttons. Otherwise it would be incosistent

@janmichek
Copy link
Collaborator

firefox_Puc6i2s3to.mp4

I think the menu should not open when going back. It should only close when going back.
Even tho it's not very logical, as a user I don't expect opening menu again when navigating back

@lukeromanowicz lukeromanowicz force-pushed the feat/support-mobile-history-back-button branch from 6c2368b to c77cfe3 Compare August 30, 2023 10:14
@lukeromanowicz
Copy link
Contributor Author

@CharisSiarampalis @janmichek new version was rolled out and I think it's now working accordingly to the expectations you stated. Let me know if it's good

Copy link
Collaborator

@janmichek janmichek left a comment

Choose a reason for hiding this comment

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

Bigup! 🎖️ nice solution, readable and not that much code!

I only found one missing case:

  1. go to dashboard
  2. open navigation
  3. click browser back
  4. you will be redirected to previous page (navigation is not just closed)

@@ -3,7 +3,7 @@
<div
:class="[
'header__container',
{ 'header__container--open': isNavigationOpen },
{ 'header__container--open': isNavigationDrawerOpen },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
{ 'header__container--open': isNavigationDrawerOpen },
{ 'header__container--open': isNavigationOpen },

I think we can stick to original naming, or is there some consideration for renaming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used NavigationDrawer just because I find it more specific and precise. Navigation is ambiguous since it may refer to bottom navigation or top navigation. Moreover, navigation on desktop is visible all the time, only the drawer is something that can be opened or closed. If this doesn't convince you I'll go back to the previous name

Copy link
Collaborator

@janmichek janmichek Sep 5, 2023

Choose a reason for hiding this comment

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

I don't see a need for renaming as nothing has fundamentally changed, but if you insist on something more precise, you can use isMenuOpen. Just wanted to avoid introducing new names to codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion, I renamed it to isMobileMenuOpen

src/app.vue Outdated Show resolved Hide resolved
src/stores/ui.js Show resolved Hide resolved
@janmichek
Copy link
Collaborator

Bigup! 🎖️ nice solution, readable and not that much code!

I only found one missing case:

1. go to dashboard

2. open navigation

3. click browser back

4. you will be redirected to previous page (navigation is not just closed)

Can you somehow imitate and pass fake history event when the navigation is open for the first time?

@lukeromanowicz
Copy link
Contributor Author

Yes, that's the only idea I got so far

@lukeromanowicz
Copy link
Contributor Author

lukeromanowicz commented Sep 6, 2023

I proposed a new version that adds "#navigation-open" hash if the page was visited for the first time to get rid of scenario that Jan mentioned. The drawback is that in this very specific scenario:

  • open new tab and visit aescan
  • open menu
  • click any of the links
  • click back button twice

to go forward you will also need to press forward button twice, since the hash stays in the history. Router history does not support detecting how much history is available ahead of the user and implementing this behavior would require at least a couple of additional hours of work and testing. I'm not even sure if it's possible to implement 100% reliably. I think the currently proposed version is the best balance of user experience to development effort.

@janmichek
Copy link
Collaborator

I proposed a new version that adds "#navigation-open" hash if the page was visited for the first time to get rid of scenario that Jan mentioned. The drawback is that in this very specific scenario:

* open new tab and visit aescan

* open menu

* click any of the links

* click back button twice

to go forward you will also need to press forward button twice, since the hash stays in the history. Router history does not support detecting how much history is available ahead of the user and implementing this behavior would require at least a couple of additional hours of work and testing. I'm not even sure if it's possible to implement 100% reliably. I think the currently proposed version is the best balance of user experience to development effort.

Ok, thanks for the new solution. I would say the previous one was a one star better than this one .) What you think?

src/utils/constants.js Outdated Show resolved Hide resolved
@lukeromanowicz
Copy link
Contributor Author

lukeromanowicz commented Sep 6, 2023

@janmichek Personally, I think the latest approach is more consistent and better for the user than the previous one. It reduces big inconsistency in very particular scenario to very small issue in that scenario (the need to press the button twice and only in very very specific scenario that may not happen ever to anyone - let's be real, who navigates back and forth on mobile device using history buttons)

@janmichek
Copy link
Collaborator

@janmichek Personally, I think the latest approach is more consistent and better for the user than the previous one. It reduces big inconsistency in very particular scenario to very small issue in that scenario (the need to press the button twice and only in very very specific scenario that may not happen ever to anyone - let's be real, who navigates back and forth on mobile device using history buttons)

Ok good, let's see Michele opinion. Thansk for fixes

Copy link
Collaborator

@janmichek janmichek left a comment

Choose a reason for hiding this comment

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

Good job 🍰

@janmichek janmichek force-pushed the feat/support-mobile-history-back-button branch from aef3c9c to 193705c Compare June 3, 2024 10:59
@janmichek janmichek closed this Jun 3, 2024
@janmichek
Copy link
Collaborator

Build stopped working. Let's continue in here #828

@janmichek janmichek deleted the feat/support-mobile-history-back-button branch June 5, 2024 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mobile menu should support back button to close it
3 participants