Skip to content
This repository has been archived by the owner on Aug 21, 2023. It is now read-only.

SideNavigation component #494

Merged
merged 45 commits into from Apr 24, 2019
Merged

SideNavigation component #494

merged 45 commits into from Apr 24, 2019

Conversation

plbabin
Copy link
Collaborator

@plbabin plbabin commented Feb 21, 2019

SideNavigation component

This update add a new SideNavigation component. The component is flexible enough to handle pretty much any situation that we have right now in HS-App. We should be able to recreate and improve all sidebar navigation.

The component can handle multiple state:

The default one
screen recording 2019-02-21 at 05 09 pm

Collapsed
screen recording 2019-02-21 at 05 11 pm

Floating menu
screen recording 2019-02-21 at 05 13 pm

@netlify
Copy link

netlify bot commented Feb 21, 2019

Deploy preview for hsds-react ready!

Built with commit 500cbb3

https://deploy-preview-494--hsds-react.netlify.com

@ItsJonQ
Copy link
Contributor

ItsJonQ commented Feb 22, 2019

@plbabin Merging in latest master for ya. It comes with Netlify and Travis performance boosts 👍

@coveralls
Copy link

coveralls commented Feb 22, 2019

Pull Request Test Coverage Report for Build 2049

  • 286 of 286 (100.0%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 2023: 0.0%
Covered Lines: 7936
Relevant Lines: 7936

💛 - Coveralls

@plbabin
Copy link
Collaborator Author

plbabin commented Feb 26, 2019

@jaredmcdaniel I added you as a design reviewer for this new UI component

src/components/SideNavigation/SideNavigation.css.js Outdated Show resolved Hide resolved
src/components/SideNavigation/SideNavigation.css.js Outdated Show resolved Hide resolved
src/components/SideNavigation/SideNavigation.css.js Outdated Show resolved Hide resolved
text-decoration: none;

&:hover {
color: ${getColor('charcoal.800')};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably on the spec, so you might have to leave it but I wanted to note that this hover effect is practically imperceptible

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no hover state on prod right now. @jaredmcdaniel do you have a prefered hover color when the heading is a link (main mailbox page for example) ?

stories/SideNavigation.stories.js Outdated Show resolved Hide resolved
@jaredmcdaniel
Copy link
Contributor

jaredmcdaniel commented Feb 27, 2019

@plbabin As far as I can tell (and maybe Im missing something glaring on my end?) it looks like we're a ways out yet. Here's the list of things I noticed on first pass:

  1. Title/Header font weight. Currently set to 500. It should be 400.
  2. I'm guessing the folder icons are just placeholders, but can we confirm we've got the right ones plugged in for the folders displayed?
  3. with dropdown header
    a) should have a caret to the right: http://c.hlp.sc/60e92e8df08f
    b) It also has an active state (blue text): http://c.hlp.sc/a56c2286f854
    c) Can we confirm the li active state in the dropdown? I'm not seeing it here.
  4. Inactive folder counts are currently set to 400 font-weight. Should be 500.
  5. with titled section
    a) can we add -webkit-font-smoothing: antialiased; to the section headers?
    b) Section headers currently have 4px bottom padding. Can we make it 6px?
  6. with regular button
    a) CTA should reflect the HSDS tertiary button styles: font weight 500, 30px tall, 15px horizontal padding: https://github.com/helpscout/hsds-product-ui/wiki/Buttons#tertiary
  7. with multiple section Shouldn't this have a title? http://c.hlp.sc/d839d5d3d1e4
  8. with footer
    a) I know this is a factor of the icons themselves, but I'd just like to confirm that we can visually center these icons as-needed http://c.hlp.sc/076c794d68a8
    b) The icon caret is pretty small: http://c.hlp.sc/45fd9f878dfd. Would like for it to be consistent with what's in production today: http://c.hlp.sc/b560d8ae530d
  9. is collapsed
    a) There should be no jumping around when the sidebar is expanded and collapsed. See production: http://c.hlp.sc/8d20447bad8c
    b) In storybook here, when the sidebar is expanded is "pushes" the content section to the right. Want to make sure it "overlays" the content when expanded.
    c) There should be a transition/animation when expanding/collapsing the sidebar vs just popping.
  10. is floating
    a) menu button toggle should be 36px x 36px and the menu icon is wrong http://c.hlp.sc/f647bc8d670c
    b) When the sidebar is visible, the toggle button should have an active state http://c.hlp.sc/093550afc564
    c) There should be no right pixel border on the sidebar, there should be a border radius on the top right and bottom right corners, and there should be a box shadow on the entire container: http://c.hlp.sc/10901c25ef5a
    d) The icons for the "footer" action items are the wrong color: http://c.hlp.sc/35256d369b8c. Should be grey-700 http://c.hlp.sc/375f29e1d7b3
  11. Do we have/need to put together a version similar to what we use for tags? Top dropdown, filters? http://c.hlp.sc/081f883b79d3
  12. On the default with header Folder view, just want to make sure we've included styles for the Needs Attention folder.

@plbabin
Copy link
Collaborator Author

plbabin commented Mar 1, 2019

thank you @jaredmcdaniel for that first pass. I should be able to tackle a lot of those points next monday

I think we should do a pass on Icons and import all svg that we have from our sketch file inside hsds-react. Or at least do the ones from the sidebar. Right now it's all placeholders, and it was to showcase the flexibility of the component

For all situation like is collapsed and is floating, I assume at first that some work would have been done inside hs-app to make it 100% perfect. Right now it's was just a demo, and not a reflection of the final result. I will follow Juan Pablo's suggestion and add some component to handle those situation inside hsds-react.

@ItsJonQ
Copy link
Contributor

ItsJonQ commented Mar 1, 2019

@jaredmcdaniel Just a heads up, gonna be 🍐 'ing with @plbabin this afternoon :)

@jaredmcdaniel
Copy link
Contributor

Following up ... again ... on #1. I was 100% wrong, and the current styling and minimal hover styling is fine. 👍

@plbabin
Copy link
Collaborator Author

plbabin commented Mar 22, 2019

Following up on #1:

I'm 99% sure the only time the header text should be a "link" is when it is a dropdown. It's never just a link that takes you to another page.

And I think I mentioned this in an earlier comment, but do we need a sidebar variation similar to the structure use for tags?

we will need one. I wonder if it should be include in this PR since it's not really a component that is needed right away. We could go with what we have in place, and then improve the component when it's required by a HS-App

thoughts?

cc @jaredmcdaniel

@knicklabs
Copy link
Collaborator

@plbabin I would not add additional functionality at this point. I think it would be best to card up that requirement on the HSDS Trello Board and we can grab it whenever is convenient.

@plbabin
Copy link
Collaborator Author

plbabin commented Apr 3, 2019

@jaredmcdaniel could you do a last design review pass when you have a chance? I think we are in a very good place for a V1 of this component

thank you!

@ItsJonQ
Copy link
Contributor

ItsJonQ commented Apr 12, 2019

@plbabin No rush! Just following up on this one 😊

Anything I can do to help? Should we put this on hold for now?

Copy link
Collaborator

@knicklabs knicklabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think we should go ahead and merge this! 🚀

@ItsJonQ
Copy link
Contributor

ItsJonQ commented Apr 24, 2019

Copy link
Collaborator

@tinkertrain tinkertrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! :shipit:

@plbabin
Copy link
Collaborator Author

plbabin commented Apr 24, 2019

@knicklabs I agree! But first.. we need to fix tests (@plbabin )

:lolcry:

https://travis-ci.org/helpscout/hsds-react/builds/520376422?utm_source=github_status&utm_medium=notification

@ItsJonQ last commit fixed all tests 😄

@ItsJonQ
Copy link
Contributor

ItsJonQ commented Apr 24, 2019

@plbabin Awesome! Will merge + release once Travis is 💚

@ItsJonQ
Copy link
Contributor

ItsJonQ commented Apr 24, 2019

@plbabin Lol. Tests are still breaking 🔥 😭

@ItsJonQ
Copy link
Contributor

ItsJonQ commented Apr 24, 2019

Will take a looksy on my end

@ItsJonQ
Copy link
Contributor

ItsJonQ commented Apr 24, 2019

@plbabin I think I got it. Not your fault... just, a weird thing that's happening with Emotion?

In the SideNavigation.css.js file, we're using nested selectors to reference ${Heading}. This should work.. except the way that Heading is rendering is slightly unconventional, since you can change the HTML selector.

The fix was just to use the .c-Heading className. Not ideal, but also not bad.

We still need to update tests to get coverage back up to 100%. The missing piece appears to be SideNavigation/Button.tsx for icon rendering, (line 44)

@ItsJonQ
Copy link
Contributor

ItsJonQ commented Apr 24, 2019

Adding that test! Will merge/release when ready

@ItsJonQ ItsJonQ merged commit 89cb785 into master Apr 24, 2019
@plbabin
Copy link
Collaborator Author

plbabin commented Apr 24, 2019

thank you for fixing the latest test!

@ItsJonQ ItsJonQ deleted the feature/SideNavigation-component branch May 8, 2019 19:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature 💅 minor ✌️ To indicate minor version bumps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants