Update Nav #22
Update Nav #22
Conversation
| </button> | ||
| <ul className="usa-nav__primary usa-accordion"> | ||
| <li className="usa-nav__primary-item"> | ||
| <Link href="/" activeClassName="usa-current"> |
There was a problem hiding this comment.
activeClassName as documented in preact-router doesn't seem to work? It just gets splatted into the dom as activeclassname="usa-current"... but the react types say that it should exist?
🤷
There was a problem hiding this comment.
Looks familiar: preactjs/preact-router#379
There was a problem hiding this comment.
Maybe we can recreate it like <Link className={path === '/' ? 'usa-current' : null }>
There was a problem hiding this comment.
Since this is top level render we don't have access to path? But we wouldn't be able to get that directly from a component either?
There was a problem hiding this comment.
Hm, that's frustrating 😕
I'd wonder if we could create our own context to hold the path, using the Router onChange prop:
At a glance, I'm not sure the best way to meaningfully tie that to a specific "path" or route component.
We could also defer this for now and handle it later.
Aside: I also realize looking at the internal implementation: The way we're using path to build a new path fragment may not always work if path is actually the path pattern and could be something like /d/:optional?/:params?.
There was a problem hiding this comment.
Yeah we may need to come back to this --- Also I noticed routing to / does not change the path... but it does change the display, unsure how to fix that right now
There was a problem hiding this comment.
Also I noticed routing to
/does not change the path... but it does change the display, unsure how to fix that right now
If I follow correctly, I think this is fixed in #26 by introducing the route handler for a "Home" route.
There was a problem hiding this comment.
We could publish new version of identity-style-guide if you want 18F/identity-design-system#249 ?
| </button> | ||
| <ul className="usa-nav__primary usa-accordion"> | ||
| <li className="usa-nav__primary-item"> | ||
| <Link href="/" activeClassName="usa-current"> |
There was a problem hiding this comment.
Looks familiar: preactjs/preact-router#379
| </button> | ||
| <ul className="usa-nav__primary usa-accordion"> | ||
| <li className="usa-nav__primary-item"> | ||
| <Link href="/" activeClassName="usa-current"> |
There was a problem hiding this comment.
Maybe we can recreate it like <Link className={path === '/' ? 'usa-current' : null }>
| <nav> | ||
| <div> | ||
| <Link href="/">Home</Link> | ||
| <header className="usa-header usa-header--extended"> |
There was a problem hiding this comment.
🤷 it's only used once and doesn't contain any logic? Happy to move if you feel strongly, will leave in place for now
No need to rush a release just for this? It'll get here eventually |
I don't mind either way. If you're not worried about it, it can wait 👍 |
we only have one page...so some of this may be overkill 😬
And mobile too