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

Vertical navbar #4859

Merged
merged 28 commits into from
Jun 15, 2020
Merged

Vertical navbar #4859

merged 28 commits into from
Jun 15, 2020

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented May 1, 2020

What type of PR is this? (check all applicable)

  • Feature

Description

  • Navbar
    • use same theme for desktop and mobile navbar
    • weird expand/collapse animation
    • review menu structure
  • Pages
    • test on different screen sizes
    • get rid of class="container" because it has fixed size and sometimes causes a horizontal scrollbar to appear test all pages with vertical navbar and fix issues
    • fix dashboard page title
    • fix add widget block on dashboard page in editing mode
    • fix issues with dashboard layout when container changes its dimensions. This generic solution also fixes cases when scrollbar appears in editing mode (old known issue) and when navbar is epanded/collapsed.
    • check and fix embed pages (visualization and dashboard)
  • Update tests
  • Cleanup

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

image

@arikfr
Copy link
Member

arikfr commented May 2, 2020

Nice! The query editor page got messed up, but others seem to be fine.

@arikfr
Copy link
Member

arikfr commented May 2, 2020

I pushed some updates to the vertical navbar and made the query editor work.

@susodapop
Copy link
Contributor

The dashboard title and refresh controls float weirdly.

Screen Shot 2020-05-04 at 10 20 34 AM

…xes known issues: navbar expand/collapse, scrollbar appears/hides)
@kravets-levko
Copy link
Collaborator Author

Restored version info block:

image
image

@kravets-levko
Copy link
Collaborator Author

Now username will be displayed after avatar when menu is expanded (when collapsed - only avatar stays visible). Tried to keep all animations consistent 😄

image

@gabrieldutra
Copy link
Member

3 points I had in mind that for me could be a little bit different (I don't have very specific suggestions 😕, but I'll bring they up in case you had any thoughts on them):

  1. How about having the text color a bit closer to white (or more prominent) for better contrast?
  2. Don't know if it's only with me, but I feel the dividers are too highlighted, not sure if it's better to remove them or just to make a softer transition.
  3. This one is rather on Ant Design, the collapse animation doesn't affect Menu Titles, so in this case, doesn't affect the profile image. I tried to see if was quick to fix, but since it was minor I eventually just left it away.

@kravets-levko
Copy link
Collaborator Author

@gabrieldutra Regarding 1 and 2 - no problem, I'll fix colors. As for 3 - I see no an easy fix for this. But as for me it doesn't look too bad. I think we can return to it later. I'll ping you when I'll update colors (I also want to clean CSS a bit)

@kravets-levko kravets-levko marked this pull request as draft June 3, 2020 16:23
@gabrieldutra
Copy link
Member

👍

I didn't notice it was now possible to mark a PR as a "Draft" after marking it as "Ready". Cool 😁

@kravets-levko
Copy link
Collaborator Author

kravets-levko commented Jun 3, 2020

That button is right below the Reviewers block in sidebar:

image

@kravets-levko kravets-levko changed the title Vertical navbar (experiment) Vertical navbar Jun 4, 2020
@kravets-levko kravets-levko marked this pull request as ready for review June 4, 2020 13:11
@kravets-levko
Copy link
Collaborator Author

@arikfr @gabrieldutra I adjusted text and dividers colors a bit, and did a cleanup stuff I wanted. I don't plan to add anything to this PR (unless you'll find any bugs), so please review it and let's merge it. Thank you!

@arikfr arikfr merged commit 0ac24e3 into master Jun 15, 2020
@arikfr arikfr deleted the vertical-navbar branch June 15, 2020 07:01
@arikfr
Copy link
Member

arikfr commented Jun 15, 2020

💅 💯

@gabrieldutra
Copy link
Member

Adding a note for later fix for fullscreen pages (query & dashboard):

The .headless styling should be updated (maybe we don't even need to add it on the body anymore).

Screen Shot 2020-06-15 at 11 37 05

@daniellangnet
Copy link
Contributor

Much love and appreciation for all your work and making this available open-source. Just wanted to share my two cents and say that I think this is a step backwards in terms of UX. Before, I could just read what the top menu items were, now I have to guess or try to remember which icon means what. I know I can expand the menu to show the labels, but why do this in the first place?

Maybe it's just us, but at least our team here frequently complains about the vertical nav in tools that we use (Intercom, Autopilot, etc) and not being able to find things easily.

Again, thanks for everything. Just wanted to share some feedback.

@arikfr
Copy link
Member

arikfr commented Jun 16, 2020

Thank you for the feedback, @daniellangnet. The main reason for this change is that usually people will have more horizontal space than vertical one. By moving the navbar to the side we get more room for the content (think of the query editor for example).

The specific issue with navigation, I think that this can be addressed by either having better icons and/or always showing the titles. Here's a sneak peek of an alternative design we're looking at:

CleanShot 2020-06-16 at 13 37 25@2x

It takes more width than the current one, but maybe be a reasonable compromise for the purpose of easier discovery.

@arikfr
Copy link
Member

arikfr commented Jun 16, 2020

Adding a note for later fix for fullscreen pages (query & dashboard):

The .headless styling should be updated (maybe we don't even need to add it on the body anymore).

.headless supposed to hide the nav bar. It doesn't anymore?

@daniellangnet
Copy link
Contributor

@arikfr Thank you for the explanation! I can see how having more vertical space could be useful on some pages, yes. Also I think the alternative design you showed would in fact help with usability 👌

@gabrieldutra
Copy link
Member

.headless supposed to hide the nav bar. It doesn't anymore?

🤔 true, I had in mind only the padding, but it used to hide .app-header-wrapper, which doesn't exist anymore. Will fix it.

andrewdever pushed a commit to andrewdever/redash that referenced this pull request Oct 5, 2020
* Vertical navbar

* Update vertical menu look and add create menu.

* Make query editor work with vertical nav.

* Dark mode

* Fix create menu & make sidebar fixed.

* Update Alert pages layout

* Update System status pages

* Update Queries and Dashboards list pages

* Update Query Source and Query View pages

* Use dark theme for mobile navbar

* Update Dashboard page: fix Add widget/textbox panel positioning

* Dashboard page: fix layout issues when container changes its size (fixes known issues: navbar expand/collapse, scrollbar appears/hides)

* Fix dashboard page sticky header (there was a 15px space above it)

* Fix embeds

* Extract desktop navbar component; move mobile navbar and its styles to ApplicationLayout folder

* Remove old app header

* Fix tests

* Restore version info block

* Make Percy capture entire page

* Make vertical navbar expand/collapse animation smoother (as it's currently impossible to disable it :-( )

* Fix misc UI issues (show Create label on expanded menu; fix some CSS; don't select items on click)

* Allow to override navbars with DynamicComponent

* Fix misc UI issues: expand/collapse button animation, menu items styles, menu expand/collapse animation

* Hide submenu arrow; show username when menu is expanded

* Refine CSS and make it more isolated; adjust colors

* Update tests

Co-authored-by: Arik Fraimovich <arik@arikfr.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants