Menu bar #47

mateoclarke · 2017-12-21T08:02:43Z

Closes #32 & Closes #21

I think this gets us most of the way there. We'll def have to refactor code around the navigation hierarchy. For example, what happens when you click a Service Type? We currently don't have an index of services by type. I think we should create an endpoint that gives us the data in a good format, but I made due with an existing endpoint.

Other things we may to add would be an event lister on the esc key and on a click on the overlay area to trigger a state change and close the menu.

We may also need to check this for accessibility. Do we want tab indexing to skip this when its not visible?

I used this method for SVG insertion https://css-tricks.com/creating-svg-icon-system-react/

# Conflicts: # src/components/Footer.js # src/css/components/Footer.scss

mateoclarke · 2017-12-23T04:34:56Z

src/App.js

@@ -22,11 +23,17 @@ class App extends Component {
    return (
      <Router>
        <div>
-          <Banner />
-          <I18nBanner />
+          <Route path="/" render={props => (


in order to give the Header component props.location, I had to wrap it in a Route component. We'll see how this may need to change in an SSR world.

mateoclarke · 2017-12-23T04:36:22Z

src/components/Navmenu.js

+    const endpoint = [
+      `${process.env.REACT_APP_CMS_ENDPOINT}/pages/`,
+      `?format=json&type=base.ServicePage&fields=topic(text)`,
+    ].join('')


do y'all know if there a better syntax for a mutli-line string?

mateoclarke · 2017-12-23T04:37:09Z

src/components/Navmenu.js

+        		parentTitle: title,
+        		children: getChildrenNavItemsByParent(title),
+        	}
+        })


All this nonsense (lines 33-46) could be removed if we had a better endpoint.

mateoclarke · 2017-12-23T04:38:42Z

src/components/Navmenu.js

+                    return (
+                      <li>
+                        <a className={this.getMenuItemClassName(`/service/${child.id}`)}
+                          href={`/service/${child.id}`}


this will need to change if when we switch to slugs instead of ids in the routing

ifsimicoded

SOOOOOOO, I don’t know a whole lot about React Routing, but here’s some feedback (UX, CSS, other stuff) that you can take or leave. I plan on doing more reading tonight/this week. Please don't feel like you need my approval to merge this. Go crazy. Thoughts on my feedback are always appreciated if you have time.
Thanks for your patience with my lengthy reviews. Here’s a GIPHt (see what I did there???)

some dude on the internet did some stuff with child routes as a nav menu pattern here which seemed pretty clean: https://scotch.io/tutorials/routing-react-apps-the-complete-guide

UX STUFF
can the menu persist in an open state, while the content below is repainted so the experience is less jarring?

Can we create links via names, so that if routes change, we only have to make updates to links in one place? I have a feeling we'll be changing these.

Some more comments inline

ifsimicoded · 2017-12-27T23:44:03Z

src/components/Navmenu.js

+
+  menuClassName = () => {
+    const baseClassName = `coa-Navmenu`;
+    return this.props.isOpen ? `${baseClassName}--open` : baseClassName;


For css convention — I was going with BEM class stacking (http://getbem.com/introduction/). So className=“base_class base_class—modifier”. NOT just className=“base_class—modifier”
Honestly, I haven’t spent a lot of time thinking/reading about what’s better here, just leaning on the folks who came up with BEM stuff to have done that heavy lifting.

yeah, I don't really have a strong opinion here. I updated for this case, but unless there is a clear benefit, i'd leave it to chef's choice.

ifsimicoded · 2017-12-27T23:45:00Z

src/components/Navmenu.js

+            })}
+          </ul>
+        </nav>
+        <div className={`coa-Navmenu__overlay${this.props.isOpen ? '' : '--hide'}`}></div>


I used the outline: 9999px solid rgba(0,0,0,0.5); trick on my modal overlay so I wouldn’t have to have multiple elements to create an overlay. Maybe this could work? Haven’t browser tested beyond chrome & safari desktop tested tho. Either way, maybe we can make generic overlay classes. Like coa-overlay, coa-overlay--center coa-overlay--left coa-overlay--right that can be used all over.

yeah, i just tried mimic what I saw the USWDS usa-overlay does. I was assuming it had good accessibility and browser coverage.

I'm open to refactoring this to for more holistic use.

ifsimicoded · 2017-12-27T23:47:18Z

src/components/Header.js

+          <div className="row">
+            <div className="col-xs-6 coa-Header__menu">
+              <span onClick={this.toggleMenu} tabIndex="0">MENU</span>
+              <span className="coa-Header__text-spacer">|</span>


Can the separator ‘|’ be a part of a :after content property so it’s not read as content? Or do we want it to be read as content?
Can we also make this more generic -- coa-text_spacer_vertical -- so we can use these all over? I have a feeling we'll have links listed like this in other places.

sounds good, putting this in helpers.scss. Is that a good place for this type of thing?

ifsimicoded · 2017-12-27T23:48:36Z

src/components/Navmenu.js

+                  { parent.children.map((child) => {
+                    return (
+                      <li>
+                        <a className={this.getMenuItemClassName(`/service/${child.id}`)}


Cursor pointer on the menu links plZZZZZ

i'm pretty sure we have that. it was missing on the "Menu" text so I added it there. Maybe that's what you meant.

ifsimicoded · 2017-12-27T23:49:29Z

src/App.js

+          <Route path="/" render={props => (
+            <section>
+              <Banner />
+              <I18nBanner />


Looks like the translation bar should always be fixed over the menu. This way the menu slide is also consistently positioned over where a user initially clicked to reveal the menu. Can we throw some CSS at this?

Looks like the translation bar should always be fixed over the menu.

Are you referring to the mockups? Because I saw inconsistencies in how sidebars get displayed in them. I don't see a good reason functionally (or visually) to always show the language bar. Ok if we ask @cthibaultatx for clarification on this one?

ifsimicoded · 2017-12-27T23:49:46Z

src/components/Navmenu.js

+  }
+
+  componentDidMount () {
+    const endpoint = [


thinking we should start defining our endpoint urls in one place, since these will likely be changing and we’ll likely only want to update these urls in one place.

cool, i'll create a file at src/constants/endpoints and import the string from there.

# Conflicts: # src/App.js # src/components/I18nBanner.js # src/css/App.scss # src/css/components/Footer.scss # src/css/components/_Footer.scss # src/css/layout/_Footer.scss

mateoclarke · 2017-12-30T06:06:05Z

@ifsimicoded, thanks for the feedback!

At first I was just going to leave the suboptimal jarring redirects until we land on what routing library we use with SSR. But when I looked into it, it wasn't too much to add. You can see what I did in the last commit.
fa27bca

I wasn't sure what you meant by...

Can we create links via names, so that if routes change, we only have to make updates to links in one place? I have a feeling we'll be changing these.

ifsimicoded · 2018-01-04T18:17:16Z

Hey when I said named routes, I meant like this: https://www.npmjs.com/package/react-router-named-routes
This way if we ever change the route for say the services page from /service/:id ... we could do it in one place, instead of updating links everywhere. Just something to think about it.

mateoclarke · 2018-01-04T19:26:01Z

ah, gotcha. yeah, I think in an ideal world, we get back slugs/paths instead of (or in addition to) ids to use for these links.

I'll think we'll have to refactor this nav code regardless once the information architecture gets more solidified. This came up yesterday briefly in a chat with @benweatherman & @cthibaultatx around page hierarchy. sounds like it'll be 3-4 levels of nesting.

Maybe we should anticipate having a Nav/menu content model in the CMS. What does Wagtail already give us there @benweatherman?

benweatherman · 2018-01-04T19:52:51Z

we get back slugs/paths instead of (or in addition to) ids to use for these links.

I think this is implementing cityofaustin/joplin#26

You can do this now via graphql (though it might change slightly)

{
  pages(slug:"dropoff") {
    edges {
      node {
        id
        content
      }
    }
  }
}

Maybe we should anticipate having a Nav/menu content model in the CMS. What does Wagtail already give us there @benweatherman?

We'd just need to develop a content model to allow authors to create the hierarchy. If we're not ready for authors to do that yet, we could generate the data via the API like the client is currently doing without allowing authors to change it until we get around to developing the model. Filed cityofaustin/joplin#49 as a placeholder. It's not currently set for MVP, but let me know if that's not right.

mateoclarke added 10 commits December 15, 2017 11:59

Set up footer

a13482a

placeholder for homepage

f986c4f

Add links and SVGs

2f1e494

I used this method for SVG insertion https://css-tricks.com/creating-svg-icon-system-react/

Merge branch 'master' into header

1e1f77b

# Conflicts: # src/components/Footer.js # src/css/components/Footer.scss

Add semi-functional i18n banner

b1d032d

make it more responsive

775711c

update language list

665e755

could we?

a0ac395

repurpose Header component, lay foundation for menu

e96d1da

Merge branch 'master' into menu_bar

c8ca6eb

mateoclarke added the wip label Dec 21, 2017

mateoclarke changed the base branch from master to header December 21, 2017 08:03

mateoclarke added 6 commits December 21, 2017 02:29

toggle menu

5e1b026

get navmenu data munging working mostly

436fbc9

list of nav items with proper classnames when active

677c9d6

Use parent/child language for nav hierarchy

1a168cc

add close icon svg, overlay, and some tabbing for a11y

67d8dee

add css animation

eda7653

mateoclarke removed the wip label Dec 23, 2017

mateoclarke added 2 commits December 22, 2017 22:33

Merge branch 'master' into header

fae7ead

Merge branch 'header' into menu_bar

ac325e7

mateoclarke commented Dec 23, 2017

View reviewed changes

mateoclarke requested review from ifsimicoded and benweatherman December 23, 2017 04:39

mateoclarke mentioned this pull request Dec 23, 2017

Header/Footer #21

Closed

3 tasks

ifsimicoded reviewed Dec 27, 2017

View reviewed changes

mateoclarke changed the base branch from header to master December 29, 2017 22:20

mateoclarke added 3 commits December 29, 2017 17:00

Merge branch 'master' into menu_bar

b5ae76b

# Conflicts: # src/App.js # src/components/I18nBanner.js # src/css/App.scss # src/css/components/Footer.scss # src/css/components/_Footer.scss # src/css/layout/_Footer.scss

pr feedback

b90d27f

Use React Router for Navmenu redirects

fa27bca

mateoclarke merged commit e866817 into master Dec 30, 2017

mateoclarke deleted the menu_bar branch December 30, 2017 06:08

benweatherman mentioned this pull request Jan 4, 2018

Return topic hierarchy for nav menu cityofaustin/joplin#49

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Menu bar #47

Menu bar #47

mateoclarke commented Dec 21, 2017 •

edited

Loading

mateoclarke Dec 23, 2017

mateoclarke Dec 23, 2017

mateoclarke Dec 23, 2017

mateoclarke Dec 23, 2017

ifsimicoded left a comment

ifsimicoded Dec 27, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 27, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 27, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 27, 2017

mateoclarke Dec 29, 2017

ifsimicoded Dec 27, 2017

mateoclarke Dec 29, 2017 •

edited

Loading

ifsimicoded Dec 27, 2017

mateoclarke Dec 29, 2017

mateoclarke commented Dec 30, 2017 •

edited

Loading

ifsimicoded commented Jan 4, 2018

mateoclarke commented Jan 4, 2018

benweatherman commented Jan 4, 2018

Menu bar #47

Menu bar #47

Conversation

mateoclarke commented Dec 21, 2017 • edited Loading

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

ifsimicoded 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

mateoclarke Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateoclarke commented Dec 30, 2017 • edited Loading

ifsimicoded commented Jan 4, 2018

mateoclarke commented Jan 4, 2018

benweatherman commented Jan 4, 2018

mateoclarke commented Dec 21, 2017 •

edited

Loading

mateoclarke Dec 29, 2017 •

edited

Loading

mateoclarke commented Dec 30, 2017 •

edited

Loading