Feature navbar #146

dallinbjohnson · 2018-04-04T04:50:28Z

Brief description

reformatted the scss.

Developer Certificate of Origin

I certify that these changes according to the Developer Certificate of Origin 1.1 as described at https://developercertificate.org/.

Sample pictures

...

Further details

...

Compoiled -> Compiled

- Filename matches format of other standard files in the repo (README, etc.) - Include title and copyright notice per the template text: - http://choosealicense.com/licenses/isc/ - https://opensource.org/licenses/isc-license - http://spdx.org/licenses/ISC.html#licenseText

TotomInc · 2018-04-04T07:21:23Z

The new navbar looks great, but I think the bottom-border on each links is a bit too big. What are your thoughts @rhyneav?

koester · 2018-04-04T08:23:56Z

The SCSS could still need a bit of cleanup. @dallinbjohnson you can further nest some selectors and use the resp mixing for the @media queries. Take a look at the &-operator to get an idea of how to nest class selectors and hovers

dallinbjohnson · 2018-04-04T23:16:57Z

I have never worked with SCSS before and I looked at the resp mixin and I am not sure how to integrate it into my code. I did everything that I understood the rest is a little confusing to me. sorry. @koester
Thank you guys for being so patient with me and helping me out.

koester · 2018-04-05T05:51:04Z

Being new or not knowing something is not a problem at all. We've all been there. When I'm at the office I'll edit this comment and explain how to use the resp mixing (or scss mixins in general) and give you some examples.

By just reading the code it's sometimes hard to see the bigger picture, especially when you're new to scss. I'll show you why this mixing exists and how it solves a common problem.

--

Ok let's dive into it. To ensure a consitent style in colors, breakpoints, sizes and general layout PaperCSS uses a special _config.scss where we defined those parameters, so everyone can use them in a new component. This allows you to design a component and automagically make it fit in with the rest of the components, just by using the $variables and @mixins the config provides. The @resp-mixin depends on a few variables.

papercss/src/core/_config.scss

Lines 66 to 70 in c395349

    
           // Responsive breakpoints 
        
           $large-screen: 1200px !default; 
        
           $medium-screen: 992px !default; 
        
           $small-screen: 768px !default; 
        
           $xsmall-screen: 480px !default;

These variables define the screen width of the most common breakpoints. These range from $large-screen: 1200px to $xsmall-screen: 480px. You could read them like this:

$large-screen: 1200px !default; // For Laptops
$medium-screen: 992px !default; // Tablets in landscape mode
$small-screen: 768px !default; // Tablets in portrait mode
$xsmall-screen: 480px !default; // Smartphones

Why did we define those? When you need a breakpoint to get your component to be responsive (just like you did with the @media screen (....) {...} declarations) we want to help you and make your life easier. When most of the components change at 768px, your navbar should probably follow that behaviour and change from a usual navigation to the mobile (burger) navigation. This helps keep a consistent style and the components feel "connected", as if they're all part of a bigger picture, even tho they may have been developed by completely different people.

So in theory you dont have to find out when your navbar should change "states", because the first time you probably want a burger navigation, instead of the usual navbar, is for tablet and smartphone devices. So you could just write something like this:

@media (max-width: $small-screen) {
  nav .collapsible .collapsible-body {
    display: contents;
  }
}

and the SCSS compiler will translate it for you to:

@media (max-width: 768px) {
  nav .collapsible .collapsible-body {
    display: contents;
  }
}

Nice! Now we dont have to remeber the 768px breakpoint everytime we need it, because we know that the $small-screen variable will give us the right value. And this value will stay consistend for all components. Also, if you later find out that you dont need your design to change at 768px but maybe at 712px you dont have to go through every component and change every media query. All you have to do is go into the _config.scss and change the value of the '$small-sceen' variable ONCE. The change will affect every component that used this variable. If that's not the coolest fucking thing ever I dont know what is. But its still very tedious to write out the media-query eeeverytime we need this. In your component it may not seem obvious, since there are not many breakpoints. But think about a bigger component with styling for multiple subnavigations and dropdown menus. This could mean that you need a lot more breakpoints and things can get out of hand very quickly. Its also not really fun to maintain such code, since one change could mean you have to edit 90 lines of media-queries.

Thaaaat's where the @resp-mixin comes in. It gives you a shorthand to quickly define what breakpoint you need and allows you to nest it into the selector that you want to change at a specific screen width. This may sound a bit complicated but you'll see what I mean. First lets take a look at the mixin:

papercss/src/core/_config.scss

Lines 129 to 138 in c395349

    
           @mixin resp($max:null, $min:null) { 
        
             @if $max == large or $max == lg  { $max: $large-screen; } 
        
             @if $max == medium or $max == md { $max: $medium-screen; } 
        
             @if $max == small or $max == sm { $max: $small-screen; } 
        
             @if $max == xsmall or $max == xs { $max: $xsmall-screen; } 
        
             @if ($min != null and $max != null) {@media only screen and (max-width: $max) and (min-width: $min) { @content; }} 
        
             @else if($max != null and $min == null){@media only screen and (max-width: $max) { @content; }} 
        
             @else if($min != null and $max == null){@media only screen and (min-width: $min) { @content; }} 
        
             @else { @error "no matching size found";} 
        
           }

This is how I read that code:

// A new mixin with the name 'resp' is defined and its arguments '$max' and '$min' are given a default value of null. That is if you include the mixin without providing any arguments, both of them will be null. If you provide just one, the other will default to null.
@mixin resp($max:null, $min:null) { 

// These if-conditions check if the first argument ($max) is either large, medium, small or xsmall or the shorter syntax lg, md, sm and xs. If you include the mixin with @include resp(large) {...} this if-condition will resolve to true. If it is true, the value of $max will change from null (the default) to $large-screen.
// In a sentance this would read as:
// Is $max equal to the string 'large' or 'lg'? If so, change the value of $max to $large-screen. If not, go on to the next line. Now lets see if $max is equal to the string 'medium' or 'md'. If so, change the value of $max to $medium-screen. If not, go on to the next.
  @if $max == large or $max == lg  { $max: $large-screen; }
  @if $max == medium or $max == md { $max: $medium-screen; }
  @if $max == small or $max == sm { $max: $small-screen; }
  @if $max == xsmall or $max == xs { $max: $xsmall-screen; }

// After those checks we check one last time if $min and $max are NOT null. This way we allow the user to use the mixin with a pixel, em or rem value for a custom breakpoint and also check if the mixin was included with atleast one parameter.
// If $min AND $max are not equal to null then the mixin will return @media only screen and (max-width: $max) and (min-width: $min) { ... }
  @if ($min != null and $max != null) {@media only screen and (max-width: $max) and (min-width: $min) { @content; }}

// If $max is not equal to null but $min is still null the mixin will return @media only screen and (max-wdith: $max) {...}
  @else if($max != null and $min == null){@media only screen and (max-width: $max) { @content; }}

// If $min is not equal to null but $max is still null the mixin will return @media only screen and (min-width: $min) { ... }
  @else if($min != null and $max == null){@media only screen and (min-width: $min) { @content; }}

// If not a single if condition before this line was met then it will throw an error.
  @else { @error "no matching size found";}
}

So how do we apply this now? Consider this simple example:

.box {
  width: 50%;
}

We have a Class with the name box and gave it a width of 50%. Now we check it in the browser and see that at 700px it would be best to change the width to 100%, to take the full width of the screen. We could do it the way you did it and throw in a @media-declaration and change the with of the class inside that. But lets use the power of scss and the resp mixin. With the mixin this could look like this:

.box {
  width: 50%;
  
  @include resp(small) {
    width: 100%;
  }
}

// CSS Output
.box {
  width: 50%;
}
@media only screen and (max-width: 768px) {
  .box {
    width: 100%;
  }
}

Doesn't that read a lot better? Just by looking at the code you know that the .box will change its width from 50% to 100% when the given screen size is met. The compiled CSS output will stay the same as to what you wrote before, but I didnt have to remember what exact value my width needed to be. I just used the variables defined in the config. This also allows for 'stacking' breakpoints. Maybe the box should be 75% for laptops? Sure:

.box {
  width: 50%;
  
  @include resp(medium) {
    width: 75%;
  }
  
  @include resp(small) {
    width: 100%;
  }
}

// CSS:
.box {
  width: 50%;
}
@media only screen and (max-width: 992px) {
  .box {
    width: 75%;
  }
}
@media only screen and (max-width: 768px) {
  .box {
    width: 100%;
  }
}

Again, just by reading the SCSS Code now it is made clear how the box will behave. The mixin allows you to set breakpoints in a fast and consistent manner and also "connect" the @media queries to the class you want to change, instead of having your class definitions at the top of the file and the media-queries at the bottom.
This mixin is "block"-scoped. So no matter how deep your rules are nested, the mixin will always correspond to the "block" it was included in. Another example:

nav {
  ul.inline {
    padding: 0;
    margin-bottom: 0;
    
    li {
      display: inline-block;
      margin: 0 .5rem;

      @include resp(small) { 
        margin: 0 1rem;
      }
      
      &:before {
        content: "";
      }
      
      a {
        font-size: 1.3rem;
      }
    }
  }
}

//CSS:
nav ul.inline {
  padding: 0;
  margin-bottom: 0;
}
nav ul.inline li {
  display: inline-block;
  margin: 0 .5rem;
}
@media only screen and (max-width: 768px) {
  nav ul.inline li {
    margin: 0 1rem;
  }
}
nav ul.inline li:before {
  content: "";
}
nav ul.inline li a {
  font-size: 1.3rem;
}

With nesting you can create a logical hierarchy and save yourself the time to write the same selector more than once.
So instead of:

nav ul.inline { ... }
nav ul.inline a { ... }
nav ul.inline h1 { ... }
nav ul.inline h2 { ... }
nav ul.inline p { ... }

You could just do

nav {
  ul.inline {
    a { ... }
    h1 { ... }
    h2 { ... }
    p { ... }
  }
}

This will give you the exact same css output above, but it is easier to read, easier to maintain and a lot faster to write. We could even take this a step further if we consider that the ul inide the nav may have more than just the .inline-class. This is where the &-operator will help us out. Usually the nesting rules in SCSS follow a hierarchy, as shown above. With the & operator we can counter this.
Example:

nav {
 ul {
   li { }
  }
}

So nav is the parent of ul and ul is the parent of li. Or the other way around li is inside of ul is inside of nav. In the DOM this would correspond to the following:

<nav>
  <ul>
    <li></li>
  </ul>
</nav>

With nesting selectors and & we can mimic this in our SCSS files. But what about a case where we dont want to go a level deeper but stay on the same level? How could we possibly say that the .inline class should not be inside of ul but on the ul element itself. This is how we do it:

nav {
 ul {
  &.inline {
    li { color: pink; }
   }
   li { color: blue }
  }
}

The &-operator treats the nested rule as if it was on the same level as its parent. This will give us the expected css:

nav ul.inline li {
  color: pink;
}
nav ul li {
  color: blue;
}

To really see what the SCSS compiler is doing to your code behind the scenes and how the SCSS code you write will translate to plain CSS I'd advice you to play around with https://www.sassmeister.com/. When I was starting to learn SCSS this was a great way to learn for me. Give it a try. Another great resource to learn SCSS is the official starter guide: http://sass-lang.com/guide

Try to apply the new things you've learned to your navbar component. I apologize for any spelling mistakes and if you still have some questions left or anything I just wrote is unclear feel free to ask.

koester · 2018-04-05T09:09:46Z

src/components/_navbar.scss

+    -webkit-transform: rotate(45deg) translate(-8px, -9px);
+    transform: rotate(45deg) translate(-8px, -9px);
+  }
+}


I can see a pattern here. Is there a way to further nest those selectors and reduce the duplicated code?

koester · 2018-04-05T09:12:08Z

src/components/_navbar.scss

+  top: 0;
+  right: 0;
+  left: 0;
+}


Different Classes for the same parent? How could you nest them together so you only have one nav with everything else nested inside?

rhyneav · 2018-04-05T11:36:35Z

@koester where can I read more of your writing?

@TotomInc I think it looks good! Maybe we can play around with different border thicknesses, but overall I like the look.

koester · 2018-04-05T12:23:41Z

I'm usually more of a reader than a writer. But for $5 I'll start a blog 😂🍻

rhyneav · 2018-04-05T21:38:53Z

@koester thoughtsofkoester.com

koester · 2018-04-09T11:41:07Z

@rhyneav Oh shieeet, thats one hell of a domain name for a blog. Now I'm really thinking of starting one. Just this small problem remains.. what the hell should I blog about? 🤔

@dallinbjohnson Do you need further help? If there are still questions left feel free to ask.

dallinbjohnson · 2018-04-10T02:55:51Z

Sorry I have not had time to work on this I am working and going to school. It's getting close to finals. I will make time to work on it though. Everything that you taught me was great it really helped me understand.

dallinbjohnson · 2018-05-20T17:02:08Z

How do I create another pull request with the changes that I have made to my feture-navbar? @rhyneav

rhyneav · 2018-05-22T15:25:15Z

You can just add additional commits to this feature-navbar branch @dallinbjohnson

If you want a fresh PR, you can copy your feature-navbar branch and open a new PR with that:

git checkout -b feature-newnavbar feature-navbar
git push origin feature-newnavbar

Then open a new PR through Github.

dallinbjohnson · 2018-05-23T16:20:34Z

what are the checks that are not successful? and is there anything else I need to change to make my code so it can be merged? @rhyneav

rhyneav · 2018-05-23T19:29:54Z

I think the checks are giving an error because of the way npm is being installed in the test runner. I'm working on the fix in another branch. I don't think they're breaking from anything you're adding, don't worry!

Before getting your code merged in, make sure you are working off of the develop branch. With this, in Github, request to merge into the develop branch (rather than master). Also, @koester gave some great advice about DRYing up your code. Could you refactor your _navbar.scss to what he recommended?

koester

Edit:
Nevermind, you already fixed it. Very good job! I didnt see your last commit before I submitted my review, so dont mind what I just wrote 🎉

dallinbjohnson · 2018-05-23T20:14:36Z

It should be fixed now! let me know if there are any more changes I need to make. @rhyneav @koester

rhyneav · 2018-05-24T12:32:12Z

Looks great @dallinbjohnson thank you for taking care of those edits! Since this PR and #145 are identical, I'll merge #145 into develop since it is already pointing at it.

Great work, very excited to have a navbar added to PaperCSS!

rhyneav and others added 6 commits March 22, 2018 18:37

Merge branch 'release-1.4.1' into develop

d16dc8b

Fixing typo.

373e495

Compoiled -> Compiled

Merge branch 'standards' into develop

c460eff

Added the html and css for the navbar

4f53ae4

Reformated my scss and added some styles to make it look better

9c8d7d9

TotomInc requested review from Fraham and rhyneav April 4, 2018 07:20

koester requested changes Apr 5, 2018

View reviewed changes

Cleaned up my code with scss and html code

720679b

Factored down even more my scss

f7f12f7

koester requested changes May 23, 2018

View reviewed changes

rhyneav mentioned this pull request May 24, 2018

Added the html and css for the navbar #145

Merged

1 task

rhyneav closed this May 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature navbar #146

Feature navbar #146

dallinbjohnson commented Apr 4, 2018 •

edited

TotomInc commented Apr 4, 2018

koester commented Apr 4, 2018

dallinbjohnson commented Apr 4, 2018 •

edited

koester commented Apr 5, 2018 •

edited

koester Apr 5, 2018

koester Apr 5, 2018

rhyneav commented Apr 5, 2018

koester commented Apr 5, 2018

rhyneav commented Apr 5, 2018

koester commented Apr 9, 2018 •

edited

dallinbjohnson commented Apr 10, 2018

dallinbjohnson commented May 20, 2018 •

edited

rhyneav commented May 22, 2018

dallinbjohnson commented May 23, 2018

rhyneav commented May 23, 2018

koester left a comment •

edited

dallinbjohnson commented May 23, 2018 •

edited

rhyneav commented May 24, 2018

Feature navbar #146

Feature navbar #146

Conversation

dallinbjohnson commented Apr 4, 2018 • edited

Brief description

Developer Certificate of Origin

Sample pictures

Further details

TotomInc commented Apr 4, 2018

koester commented Apr 4, 2018

dallinbjohnson commented Apr 4, 2018 • edited

koester commented Apr 5, 2018 • edited

koester Apr 5, 2018

Choose a reason for hiding this comment

koester Apr 5, 2018

Choose a reason for hiding this comment

rhyneav commented Apr 5, 2018

koester commented Apr 5, 2018

rhyneav commented Apr 5, 2018

koester commented Apr 9, 2018 • edited

dallinbjohnson commented Apr 10, 2018

dallinbjohnson commented May 20, 2018 • edited

rhyneav commented May 22, 2018

dallinbjohnson commented May 23, 2018

rhyneav commented May 23, 2018

koester left a comment • edited

Choose a reason for hiding this comment

dallinbjohnson commented May 23, 2018 • edited

rhyneav commented May 24, 2018

dallinbjohnson commented Apr 4, 2018 •

edited

dallinbjohnson commented Apr 4, 2018 •

edited

koester commented Apr 5, 2018 •

edited

koester commented Apr 9, 2018 •

edited

dallinbjohnson commented May 20, 2018 •

edited

koester left a comment •

edited

dallinbjohnson commented May 23, 2018 •

edited