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

Navigation button is hard to see #1117

Closed
christianlupus opened this issue Jul 26, 2022 · 13 comments · Fixed by #1122
Closed

Navigation button is hard to see #1117

christianlupus opened this issue Jul 26, 2022 · 13 comments · Fixed by #1122
Assignees
Labels
Frontend Issue or PR related to the frontend code question Further information is requested

Comments

@christianlupus
Copy link
Collaborator

When looking at the current state of #1105, the navigation menu is reappearing again. See below:
grafik

When the navigation is expanded and the zoom factor is set correctly, the button might overlap with the edit (or other) button(s).

At least for me, this is almost invisible. I have seen this problem already in the past also with other apps on my mobile that the styling of the nav buttons might conflict with underlying content. Graphical example:

grafik

I am not sure if this is really an app issue or better suited in the Nextcloud style guides (as other apps are affected similarly).

@christianlupus christianlupus added question Further information is requested Frontend Issue or PR related to the frontend code labels Jul 26, 2022
@MarcelRobitaille
Copy link
Collaborator

MarcelRobitaille commented Jul 26, 2022

I have noticed this as well. This is caused by @nextcloud/vue AppNavigation, and they don't provide a clean way to solve this IMO. I dislike many things about AppNavigation to be honest. I find the placement of the button over our own titlebar using position: absolute to be hacky. I think the close button should be inside the sidebar so that they can ensure good contrast and that it's not overlapping with the app's elements. I wish they would let us implement our own open button that we can position in a sane way using flexbox in our title bar and a close button that they position in a sane way in the sidebar.

As for a workaround, we could put an overlay over the app when the sidebar is open. Another gripe about the AppNavigation is that there's no good way to know if it's open. They document how to open and close it using event-bus, but I was not able to subscribe to those events after reading through the event-bus docs. Maybe we could hack something together in css with a sibling selector on app-navigation--close. However, the sidebar has 4 states: widescreen shown, widescreen manually dismissed, narrow screen hidden, and narrow screen manually opened. The sibling selector would only detect if it's closed or not. Maybe we could use a media query as well, but then we would have a magic number that could be changed upstream.

@MarcelRobitaille MarcelRobitaille self-assigned this Jul 26, 2022
@christianlupus
Copy link
Collaborator Author

Yes, that is the issue. The problem lies in the Vue library, I see. This problem is only a minor issue at the moment. So, I would rather check for a solid solution before going into some hackery.

So, the question for @nextcloud/vuejs would be if a PR would be accepted/anticipated that will add emitting events to the AppNavigation component for showing/hiding the navigation component. If yes, I would suggest that we are forking the repo, add the code and create a PR.

@christianlupus
Copy link
Collaborator Author

Well, I was able to tap into the event bus. I will send you a PR.

@MarcelRobitaille
Copy link
Collaborator

Really? I just tested it again and I am not getting any events. Did you have to do anything special? I copied the same subscribe call as AppNotification and nothing.

@christianlupus
Copy link
Collaborator Author

@MarcelRobitaille
Copy link
Collaborator

What the heck. I was subscribing to toggle-navigation. I didn't notice that they were emitting navigation-toggled instead. Thanks!

@christianlupus
Copy link
Collaborator Author

Yes, the same happened to me. The trick is that with toggle-navigation you can simulate a clock on the toggle button.

@seyfeb
Copy link
Collaborator

seyfeb commented Jul 27, 2022

There is a vue-devtools browser plugin which should record emitted events and might help detecting the event names!

@MarcelRobitaille
Copy link
Collaborator

The plugin has not been working reliably for me. Sometimes it shows up, but sometimes it doesn't recognize that it's a vue app.

@seyfeb
Copy link
Collaborator

seyfeb commented Jul 27, 2022

I had this problem, too. Switching to a different browser helped with that. (I think the Firefox extension was buggy and Chrome worked well. Maybe the other way round)

@christianlupus
Copy link
Collaborator Author

christianlupus commented Jul 27, 2022

Yes and no. Firefox is not working ATM. Chrome works well, but you need to do some steps in the right order:

  • Make sure the dev tools are closed.
  • Make sure, you have a dev build and force-relaod (Ctrl + F5)
  • Click on the Vue symbol. A message will appear that Vue is not detected. Wait 2~3 secs then close the message popup
  • Open the dev tools
  • Be patient, it takes 1~3 seconds until the Vue tab shows up.

The problem is that here, the event-bus is used instead of plain old Vue events. These events will not be detected by the Vue browser extension.

@seyfeb
Copy link
Collaborator

seyfeb commented Jul 27, 2022

Ah sorry then my comment was rather misleading 🙊

@MarcelRobitaille
Copy link
Collaborator

Thanks for the instructions @christianlupus. I thought it would "just work". Not sure why it was sometimes working because I never did all of that. I am getting stuck on # 3 though. I don't have any confirm button, just the message "Vue.js is not detected". If I skip that step, I don't get any Vue tab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Issue or PR related to the frontend code question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants