Add the new navbar and refactor the sidebar #109

morajabi · 2017-09-06T15:19:48Z

First, I'm sorry for latency, I was in the middle of a heavy project.

I think it's now complete (check designs), so take a look and tell me what you think.
I know some parts can be refactored so don't merge, please :)

We can start implementing search after merging this.

Thanks for your time and help.

…or mobile

SaraVieira · 2017-09-06T15:26:58Z

Deploying this :)

morajabi · 2017-09-06T15:27:18Z

I forgot to push my changes :)

SaraVieira · 2017-09-06T16:14:07Z

@morajabi @mxstbr @philpl Deploy: https://styled-components-docs-avyawrdzzh.now.sh/

mxstbr

This is so so awesome, great great great work! I can't wait to ship this 😊

A couple of minor improvements:

When I'm on e.g. /docs/basics all the other sections in the NavBar should be collapsed:

The social media icons should have the same hover animation/transitions as the other menu items
Let's comment out the Twitter icon for now since styled-components doesn't have a Twitter account (yet?)
Let's comment out the search input since we don't have search yet?

morajabi · 2017-09-06T17:53:25Z

@mxstbr Your comment is beyond my expectations :) Thanks, Max! I wanted to make it solid both in code and design, and hopefully, it is so.

Your points are completely right and I agree too. If it's OK, I will do them tomorrow.

…ve one

…ter)

morajabi · 2017-09-11T16:19:54Z

@philpl

They're overlapping. Having two floating menus open at the same time is something I'd generally consider as bad practice and kind of "lazy". I think we can close one when the other is opened, which is just as valuable and makes sure that the user can always focus on a single one. Also I think their order is fixed, so that means a specific one will always overlap the other. I haven't checked now which one overlaps which, but this makes a really annoying experience.

I agree with you then 👍 We need to this.

kitten

In general I have three nits:

commented out code should be removed and the git history should be relied upon instead. And if needed a TODO comment should be added instead
All the svg should be abstracted and extracted, and the very generic selector that is targeting them should be moved to them. You can probably just create a styled-svg component and change width/height
Wrapping structural components in another presentational styled() wrapper should be avoided for refactorability

kitten · 2017-09-12T13:09:55Z

components/Anchor.js

@@ -13,7 +13,7 @@ const InvisibleAnchor = styled.div.attrs({
  visibility: hidden;
  height: 0;

-  top: ${rem(-20)};
+  top: ${rem(-80)};

  ${mobile(css`
    top: ${rem(-90)};


Have you checked whether this needs some changes on mobile?

Oh, No. I'll check.

It's now good on both mobile and desktop IMO.

kitten · 2017-09-12T13:10:34Z

components/DocsLayout.js

-      </Title>
+  onSideFold = () => {
+    let isSideFolded = !this.state.isSideFolded
+    // if(!isSideFolded) {


Can you remove the commented out code here? If you need it back we'll have the git history anyway 😉

Sorry! It was my pre-git habit. You won't see this kind of commenting later on.

No worries! I sometimes forget this stuff too :)

kitten · 2017-09-12T13:11:35Z

components/DocsLayout.js

+    //   document.body.classList.remove('sticky')
+    // }
+    this.setState({
+      isSideFolded: isSideFolded


Small nit, can you remove the variable above? Also it won't need to be a let. We generally use the const before let convention

Yeah, you're right, I forgot to refactor the old code here. Sorry!

kitten · 2017-09-12T13:12:34Z

components/Nav/MobileNavbar.js

+import Link from '../Link'
+import NavLinks from './NavLinks'
+import Social from './Social'
+// import Search from './Search'


to be removed

kitten · 2017-09-12T13:13:03Z

components/Nav/MobileNavbar.js

+  padding: 0 ${rem(20)};
+  transition: height 0.1s;
+
+  -webkit-user-select: none;


user-select is prefixed only for webkit here. The prefix can be removed

Good catch!

kitten · 2017-09-12T13:14:47Z

components/Nav/MobileNavbar.js

+  `}
+`
+
+const PaddedSocial = styled(Social)`


I'm not too happy about wrapping the Social component here, since it's our own :/

It goes against the container -> structural -> presentational components order

You're totally right. I didn't think of this before container -> structural -> presentational but it makes sense for me, too.

kitten · 2017-09-12T13:15:09Z

components/Nav/MobileNavbar.js

+        <SecondaryMenu open={isMenuOpen}>
+          <NavLinks />
+          <NavSeparator />
+          {/* <Search /> */}


to be removed due to git history; see above

kitten · 2017-09-12T13:16:03Z

components/Nav/NavButton.js

+  }
+
+  svg {
+    display: inline-block;


I'd much rather have this be placed with the SVGs themselves. Surely we can choose an abstraction component for them instead of just chugging them into MobileNavbar.js

kitten · 2017-09-12T13:16:31Z

components/Nav/Navbar.js

+import Link from '../Link'
+import NavLinks from './NavLinks'
+import Social from './Social'
+// import Search from './Search'


to be removed

kitten · 2017-09-12T13:16:42Z

components/Nav/Navbar.js

+          </StartWrapper>
+
+          <EndWrapper>
+            {/* <Search /> */}


to be removed

mxstbr · 2017-09-14T20:01:02Z

We should also link to our medium publication from the navbar! https://medium.com/styled-components

morajabi · 2017-09-14T20:22:26Z

Oh, how did I miss that?! Sure.

LorbusChris · 2017-09-24T14:56:26Z

@morajabi the nav ui looks & feels excellent!

Have you thought about making these styles redistributable as a package?
I guess it would come in handy to many (like me 😃) and be worth the other ⭐ or 🌟
Don't feel pressured into taken that oss burden though, one can easily fork away from here, too!

Keep up the good work!

kitten · 2017-09-25T10:51:59Z

@morajabi Anything I can do to get this merged soon 😉 Should I pick up my nits and just go fix them? Or do you have time soon. Sorry for being pushy, but it's really awesome and we should get this merged soon ❤️

morajabi · 2017-09-25T12:26:04Z

@philpl I'm really sorry for this. I've been busy with my new startup these days, but I'm doing this right now. Again, I'm sorry and I'll fix these issues right now.

kitten · 2017-09-25T13:15:18Z

@morajabi No worries 👍 Let's get this merged and iterate on it later. Thanks again, it's really some impressive piece of UI 💟

morajabi · 2017-09-25T13:22:06Z

It's almost done now, wait 1 minute :)

morajabi · 2017-09-25T13:32:00Z

@philpl @mxstbr I think I've fixed issues addressed here! Please take a look :) https://styled-components-docs-vwoqqgftqx.now.sh/docs

mxstbr · 2017-09-25T13:37:48Z

So awesome 😍 Thanks so much @morajabi!

kitten · 2017-09-25T13:42:42Z

Ok, only thing is that we should update the navbar items 😄 We can do that after merging, I guess

morajabi · 2017-09-25T13:44:31Z

@philpl Sounds good!

morajabi · 2017-09-25T13:46:42Z

Thank you too @mxstbr and @philpl. I've learned so much from you! Sorry again for the latency...

@mxstbr P.S I love the spectrum.chat UI, libraries and building blocks. Just saw the @withspectrum OSS projects on Github, great work there!

kitten · 2017-09-25T13:53:10Z

@mxstbr do you want to approve and merge? We can subsequently open more issues with iterations on the navbar

mxstbr · 2017-09-25T14:18:26Z

Just saw the @withspectrum OSS projects on Github, great work there!

Lots more coming soon 😎

Great work @morajabi, want to deploy this @philpl?

kitten · 2017-09-25T16:07:45Z

@mxstbr Let's hold up until we figure out what to put in the navbar? :)

morajabi added 10 commits September 2, 2017 19:31

fix: remove Head

283b22a

feat: new sidebar

27ef848

refactor: use sizes variables

8aaf806

feat(nav): add new logo

9424e8b

feat(nav): add new basic navbar to top

c6be883

feat(nav): add nav links

a2c73ec

feat(nav): add social icons

4dcef97

feat(nav): add search bar

5afb593

fix(nav): use currentColor for search box colors

d038838

feat(nav): add mobile view and add folding side and displaying menu f…

cddc75b

…or mobile

morajabi changed the title ~~Add the new navbar and refactor the sidebar~~ Add the new navbar and refactor the sidebar (closes #83) Sep 6, 2017

morajabi requested review from kitten, SaraVieira and mxstbr September 6, 2017 15:22

mxstbr had a problem deploying to PR staging September 6, 2017 15:25 Error

morajabi added the enhancement label Sep 6, 2017

mxstbr requested changes Sep 6, 2017

View reviewed changes

morajabi added 8 commits September 6, 2017 22:24

fix(nav): add unified transition effect to SocialLinks

2d4b2cc

test(nav): update tests

2b805d3

fix(nav): comment out Twitter and Search for now

570d1e4

fix(nav): add Social again and remove Search instead (fix a mistake)

f2eac0c

test(nav): update snapshots

9c34064

fix(nav): remove body stickying on side fold

912a395

feat(nav): make all subsections of sidebar hidden except for the acti…

c142736

…ve one

feat(nav): add hrefs to nav menu links for now (need to be changed la…

baea209

…ter)

morajabi self-assigned this Sep 10, 2017

kitten requested changes Sep 12, 2017

View reviewed changes

kitten changed the title ~~Add the new navbar and refactor the sidebar (closes #83)~~ Add the new navbar and refactor the sidebar Sep 25, 2017

morajabi added 4 commits September 25, 2017 16:04

fix(nav): optimize anchor top space for both mobile and desktop

5c7b4ec

fix(nav): remove unnecessary commented codes

8dc4760

fix(nav): extract SVG icons and refactor some styles

7404612

feat(nav): add medium pub link

b04d1ce

fix(nav): allow only one of side or mobile nav to be open at once

654e42e

mxstbr had a problem deploying to PR staging September 25, 2017 13:28 Error

Merge branch 'master' into add-nav

4665c72

kitten approved these changes Sep 25, 2017

View reviewed changes

mxstbr had a problem deploying to PR staging September 25, 2017 13:42 Error

mxstbr approved these changes Sep 25, 2017

View reviewed changes

kitten merged commit a53e606 into master Sep 25, 2017

mxstbr deleted the add-nav branch September 25, 2017 16:10

Add the new navbar and refactor the sidebar #109

Add the new navbar and refactor the sidebar #109

Conversation

morajabi commented Sep 6, 2017 • edited by kitten

SaraVieira commented Sep 6, 2017

morajabi commented Sep 6, 2017

SaraVieira commented Sep 6, 2017

mxstbr left a comment

Choose a reason for hiding this comment

morajabi commented Sep 6, 2017

morajabi commented Sep 11, 2017 • edited

kitten 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxstbr commented Sep 14, 2017

morajabi commented Sep 14, 2017

LorbusChris commented Sep 24, 2017 • edited

kitten commented Sep 25, 2017

morajabi commented Sep 25, 2017

kitten commented Sep 25, 2017

morajabi commented Sep 25, 2017

morajabi commented Sep 25, 2017

mxstbr commented Sep 25, 2017

kitten commented Sep 25, 2017

morajabi commented Sep 25, 2017

morajabi commented Sep 25, 2017 • edited

kitten commented Sep 25, 2017

mxstbr commented Sep 25, 2017

kitten commented Sep 25, 2017

morajabi commented Sep 6, 2017 •

edited by kitten

morajabi commented Sep 11, 2017 •

edited

LorbusChris commented Sep 24, 2017 •

edited

morajabi commented Sep 25, 2017 •

edited