feat: Streamline navbar #1550
feat: Streamline navbar #1550
Conversation
|
@lidel and/or @rafaelramalho19, would you mind taking a look? Haven't touched the welcome page yet; plan to do that under a separate PR for division of labor. Unfortunately the build workflow in CircleCI is currently failing ... am a bit stuck as to why, would appreciate any guidance! Thanks 😊 |
There was a problem hiding this comment.
LGTM, (subjectively) looks and reads better.
Remaining work is to make E2E tests pass:
- E2E tests fail because of
SelectorNotFounderror occurring on Files screen- this looks like a left-over of removed width toggle logic – removing the need for
selectNavbarWidthfromFilesList.jsshould fix it
- this looks like a left-over of removed width toggle logic – removing the need for
ps.
Also replaces tab title dashes with | characters for consistency with other sites in our ecosystem
This could be a separate PR, but fair enough ;))
|
Thanks @lidel and @rafaelramalho19 -- have re-requested reviews. All comments should be taken care of removing Is this something you might have time to talk through at some point -- and would it be better to do that as a separate PR? AFAIK these changes aren't making things worse than current-state for Safari users. |
|
@lidel -- please feel free to merge if you're good. Thanks! |
src/navigation/NavBar.js
Outdated
| <span className={`dib dtc-l v-mid ${open ? 'pl3 pl5-l' : 'ph3'}`} style={{ width: 50 }}> | ||
| <a href={disabled ? null : href} className={anchorClass} role='menuitem' title={children}> | ||
| <div className='db ph2 pv1'> | ||
| <div className='db'> | ||
| <Svg width='50' className={svgClass} /> |
There was a problem hiding this comment.
Forgot about this, we should do role="img" aria-labelledby={children}
There was a problem hiding this comment.
Better yet, role="presentation"
There was a problem hiding this comment.
👍 Added role='presentation' to divs surrounding icons: see lines 40 and 60. Is this what you're thinking?
There was a problem hiding this comment.
@rafaelramalho19 - per our chat moved these directly into the img tags. Look good to you?
There was a problem hiding this comment.
It's perfect, thank you 😄
There was a problem hiding this comment.
LGTM
- cube interaction makes more sense and follows what we recently did in IPFS Companion
- it looks more "natural" than the old version, I suspect most of users may not even notice anything changed (which is a good thing!)
@rafaelramalho19 you the boss here, merge when you want ;)
This PR streamlines the navbar at both mobile/tablet and desktop widths and prepares for improving "welcome" screen content (ref #1463):
Screenshots ...
Mobile 320px wide:
Desktop:
