Update Navigation & Search #794

carlfairclough · 2020-03-04T22:45:36Z

Updated navigation & search components

Description

A pretty in-depth update of navigation and search. Involves modal-style overlays and navigation within the menu sidebar. It also involves extras

Removed navigation duplication from the Outline
No separate sidebar component
All navigation display is managed within CSS, with visibility props being managed via CSS
Search edge states catered for
- Blank
- No results

Related Issue

#764 #755

Screenshots (if appropriate):

Desktop

Nav Items

Search

Results

Blank States

Mobile

carlfairclough · 2020-03-04T22:52:11Z

@samajammin here's a draft PR on this. It's not ready to be merged IMO because the way nav links are being styled is pretty messy and should be done inside the NavLink.vue component.

I'm pausing here because it's a huge change already & I would appreciate review to catch any glaring errors before I go and make that CSS manageable.

I'm in particular looking for attention around how I'm managing properties/data with $emit

samajammin

Nice work! Overall this looks great. Left a bunch of comments on ways I think this can be cleaned up. Let me know if you have questions.

docs/.vuepress/theme/Layout.vue

docs/.vuepress/theme/components/NavLinks.vue

docs/.vuepress/theme/components/Navigation.vue

docs/.vuepress/theme/Layout.vue

samajammin · 2020-03-05T15:36:39Z

docs/.vuepress/theme/components/SearchBox.vue

+    isSearchFocused: function() {
+      // watch it
+      this.isSearchFocused &&
+        document.getElementById('main-search-field').focus()


Could we solve this with CSS instead? E.g. adding a conditional class :class="{ focused: isSearchFocused }"

This isn't a css thing, this targeting the input field:

User taps "search" on mobile

Search opens, with the input focused and as a result their keyboard opens.

I've had to switch to a display: none toggle for the search, which introduces issues with this anyway as it prevents the field from being focused. As a result I've stripped all of this out.

On a related note, something like :class="{ focused: isSearchFocused }" is now being used. The css focus-within selector isn't incredibly well supported so JS is handling this again (without significant changes):

searchClasses() { return { 'search-box': true, 'search-hidden': !this.isSearchVisible, 'focus-within': this.focused } },

samajammin · 2020-03-05T15:39:48Z

docs/.vuepress/theme/components/SearchBox.vue

+      <icon name="search" class="icon-search-field" />
+    </div>
+
+    <div v-if="blankState && isSearchFocused" class="blank-state">


Should this only display if search is focused? Seems we could display this whether or not the search input is focused.

This was a desktop-first approach and has been changed to be hidden with CSS instead when the search isn't focused

samajammin · 2020-03-05T15:41:40Z

docs/.vuepress/theme/components/NavLinks.vue

@@ -35,35 +135,222 @@ export default {
    }
  },
  methods: {
-    isActive
+    isActive,
+    handleSearchToggle(value) {


Similar to handleNavToggle, I think this could be simplified to just toggle a boolean without passing a value.

I'd prefer to keep this, it allows us to clear the search in certain scenarios i.e. when a link is clicked. imo it's much better to be explicit when there's a desired behaviour

I don't follow - it allows us to clear the search in certain scenarios i.e. when a link is clicked - where are we doing this? From what I understand this merely sets isSearchVisible to true or false.

Switched this out for the toggle like you mentioned — I honestly prefer being super explicit but I'll go with your judgement

samajammin · 2020-03-05T15:41:48Z

docs/.vuepress/theme/components/NavLinks.vue

+    handleSearchToggle(value) {
+      this.isSearchVisible = value
+    },
+    handleFocusToggle(value) {


Similar to handleNavToggle, I think this could be simplified to just toggle a boolean without passing a value.

docs/.vuepress/theme/components/SearchBox.vue

samajammin · 2020-03-05T15:51:04Z

Another styling note - let's ensure the search doesn't overlap the sidebar (i.e. keep the current functionality).

Current:

Your PR:

samajammin · 2020-03-05T15:54:38Z

A couple style notes on the desktop nav.

The nav icon is now too small & no longer center aligned
The links feel awkwardly far apart

Style guide:

Your PR:

samajammin · 2020-03-05T16:39:12Z

@carlfairclough what is the Outline you're referring to in the PR description?

carlfairclough · 2020-03-12T22:26:10Z

Another styling note - let's ensure the search doesn't overlap the sidebar (i.e. keep the current functionality).

I'm not sure I agree with this, i don't think this negatively impacts anything and is impacted by the icons/text to the right

On the focus state & classes: I'm exploring this, it focuses/tabs fine with the @blur function is removed, but then escaping the view becomes difficult (either detect a click outside the element, which would introduce complexity or a slight restructure of the search which matches the dropdown, relying on :focus-within with CSS. I think :focus-within is the best option cause we can then remove some of the JS (i.e. isSearchFocused)

carlfairclough · 2020-03-13T09:58:29Z

I believe this is ready for merge @samajammin. I've addressed most of your comments & replied to the ones which I haven't actioned with explanation

carlfairclough · 2020-03-13T11:47:40Z

I've been testing the search in Safari (desktop) and come across an issue where blur prevents the @click, @mousedown, @mouseup events from firing, whether that's on the links themselves (@blur) or the wrapper (@focusout). I'm at a loss as to the cause here, and every solution seems to surface an issue in behavior (e.g. focus not firing).

The last resort would be to only show results when the input is focused (existing behaviour) but imo that is less than ideal, so any help would be massively appreciated.

samajammin

A few minor simplifications I'd like to make but approving this as-is. Great work here! This is a excellent improvement 🔎 🎉

samajammin · 2020-03-15T03:00:48Z

docs/.vuepress/theme/components/NavLinks.vue

@@ -35,35 +135,222 @@ export default {
    }
  },
  methods: {
-    isActive
+    isActive,
+    handleSearchToggle(value) {


I don't follow - it allows us to clear the search in certain scenarios i.e. when a link is clicked - where are we doing this? From what I understand this merely sets isSearchVisible to true or false.

samajammin · 2020-03-15T03:17:19Z

docs/.vuepress/theme/components/Navigation.vue

+    <NavLinks
+      :isDarkMode="isDarkMode"
+      :isMobileNavVisible="isMobileNavVisible"
+      :handleNavToggle="handleNavToggle"


Right but your passing handleNavToggle to the NavLinks component, which doesn't do anything with that prop. I removed this line locally & it doesn't seem to change any
functionality.

docs/.vuepress/theme/components/Navigation.vue

samajammin · 2020-03-15T03:29:23Z

@carlfairclough approved! I'll merge this after you rebase against the latest dev branch. Holler if you want some quick assistance on that.

samajammin · 2020-03-15T03:31:33Z

@carlfairclough - regarding your comment:

I've been testing the search in Safari (desktop) and come across an issue where blur prevents the @click, @mousedown, @mouseup events from firing, whether that's on the links themselves (@blur) or the wrapper (@focusout). I'm at a loss as to the cause here, and every solution seems to surface an issue in behavior (e.g. focus not firing).

So what's the impact from the perspective of a user here?

Edit

Oh, is this the issue where we've lost the styling on the focused search item? This is definitely a showstopper I forgot to mention. Here's an example - I arrow'd down & selected enter on the 2nd option but the UI gives no clues to this: https://share.getcloudapp.com/NQuZlzL9

samajammin

Per the comment above, we need to fix the focus styles when navigating search results with the keyboard before merging this.

carlfairclough · 2020-03-16T18:57:50Z

I think we're good to go unless there are any blunders @samajammin

samajammin

Getting close!

2 things I immediately notice when testing the search (see screen recording):

Navigating search results with my keyboard should highlight the selected/focused option
Hitting 'enter' key to navigate to my selection should close the search

Once those are fixed I think we're good to go!

…en link clicked

carlfairclough · 2020-03-17T15:51:31Z

@samajammin resolved the Safari issue & can tab (tab key) through search results

samajammin · 2020-03-17T16:15:50Z

@carlfairclough awesome!!! Please resolve the merge conflicts & I'll work to get this merged ASAP.

samajammin

Bravo 👏

carlfairclough added 8 commits February 28, 2020 15:48

renamed Header to Navigation

afea7be

using css for navigation (desktop). Functional, untidy [WIP]

f0394c6

removed superfluous sidebar stuff. added search states

f7d2290

search dark mode & light mode for mobile

d9142e4

sidebar colors

2f906c1

fixed nav background colors

6db59b3

updated search & nav. Functional.

e846805

merged conflicts. up-to-date with dev

9f3921f

carlfairclough changed the title ~~Update Navigation & Search~~ [WIP] Update Navigation & Search Mar 4, 2020

carlfairclough changed the title ~~[WIP] Update Navigation & Search~~ Update Navigation & Search Mar 4, 2020

samajammin suggested changes Mar 5, 2020

View reviewed changes

carlfairclough added 7 commits March 12, 2020 16:13

reintroduce smooth scroll

bc75b10

renamed navdropdown file

044c01a

reintroduce smooth scroll to Layout.vue

ddcb9db

removed clickedItem function

646d841

removed superfluous function

a15688d

removed reset class

2608399

remove superfluous searchbox import

4e6dfd1

carlfairclough added 7 commits March 12, 2020 20:16

Search can now be navigated with keyboard

bb0bd15

tidy up nav spacing for desktop

5ab9efc

removed superfluous Header.vue file

e0594df

enabled keyboard navigation without :focus-within

75c2d47

Removed search auto-focus on mobile

80198de

enabled dropdown keyboard navigation across browsers. cleaned file.

0ee5bfa

Hide menu icon when menu is open. Added nav tag

6564781

carlfairclough requested a review from samajammin March 13, 2020 09:31

hide nav & search when a search link is clicked

ff7c7cf

samajammin approved these changes Mar 15, 2020

View reviewed changes

samajammin suggested changes Mar 15, 2020

View reviewed changes

carlfairclough added 4 commits March 15, 2020 23:57

merged dev into branch

cd132e9

brought over type styles to search

e8eddea

Added focus states to nav, blur input with esc

c074797

switched nav & search toggles to be actual toggles

8eeabc6

samajammin suggested changes Mar 16, 2020

View reviewed changes

Search working in safari (desktop), prevent overflow being toggled wh…

8aba374

…en link clicked

carlfairclough and others added 3 commits March 17, 2020 13:24

resolve conflicts

a462155

close search on enter

1a7aafb

Merge branch 'dev' into update-nav-component

d66f323

samajammin approved these changes Mar 17, 2020

View reviewed changes

samajammin merged commit fae93b3 into ethereum:dev Mar 17, 2020

carlfairclough deleted the update-nav-component branch March 18, 2020 03:23

samajammin mentioned this pull request Mar 20, 2020

Deploy v1.3.0 #851

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Navigation & Search #794

Update Navigation & Search #794

carlfairclough commented Mar 4, 2020

carlfairclough commented Mar 4, 2020 •

edited

Loading

samajammin left a comment

samajammin Mar 5, 2020

carlfairclough Mar 13, 2020

samajammin Mar 5, 2020

carlfairclough Mar 13, 2020

samajammin Mar 5, 2020

carlfairclough Mar 13, 2020

samajammin Mar 15, 2020

carlfairclough Mar 16, 2020

samajammin Mar 5, 2020

samajammin commented Mar 5, 2020

samajammin commented Mar 5, 2020

samajammin commented Mar 5, 2020

carlfairclough commented Mar 12, 2020

carlfairclough commented Mar 13, 2020

carlfairclough commented Mar 13, 2020 •

edited

Loading

samajammin left a comment

samajammin Mar 15, 2020

samajammin Mar 15, 2020

samajammin commented Mar 15, 2020

samajammin commented Mar 15, 2020 •

edited

Loading

samajammin left a comment

carlfairclough commented Mar 16, 2020

samajammin left a comment •

edited

Loading

carlfairclough commented Mar 17, 2020

samajammin commented Mar 17, 2020

samajammin left a comment

Update Navigation & Search #794

Update Navigation & Search #794

Conversation

carlfairclough commented Mar 4, 2020

Description

Related Issue

Screenshots (if appropriate):

Desktop

Nav Items

Search

Mobile

carlfairclough commented Mar 4, 2020 • edited Loading

samajammin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samajammin commented Mar 5, 2020

samajammin commented Mar 5, 2020

samajammin commented Mar 5, 2020

carlfairclough commented Mar 12, 2020

carlfairclough commented Mar 13, 2020

carlfairclough commented Mar 13, 2020 • edited Loading

samajammin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samajammin commented Mar 15, 2020

samajammin commented Mar 15, 2020 • edited Loading

Edit

samajammin left a comment

Choose a reason for hiding this comment

carlfairclough commented Mar 16, 2020

samajammin left a comment • edited Loading

Choose a reason for hiding this comment

carlfairclough commented Mar 17, 2020

samajammin commented Mar 17, 2020

samajammin left a comment

Choose a reason for hiding this comment

carlfairclough commented Mar 4, 2020 •

edited

Loading

carlfairclough commented Mar 13, 2020 •

edited

Loading

samajammin commented Mar 15, 2020 •

edited

Loading

samajammin left a comment •

edited

Loading