Accordion Card w/ SASS & BEM

Community feedback

• 

  Steven Toben•750

  @steventobenPosted almost 4 years ago

  Yeah I'm a little confused by some of your styling and a few elements in your dom tree. First off to address your issue about the slight scroll, it's because you haven't done a margin reset. Really, the only style you should be setting is font-size=100% so you can set your root font size. Try just setting margin to 0 on your body. Or html since for some reason your body element isn't fullscreen. I personally don't think using transforms on the body element is a good idea. I'm not too sure why you made the body a flex container, it just holds your main which is just a wrapper around your main card, and also your footer element so it seems pretty pointless to add the styling to your body element.

  Also I agree with Grace, a 12px font is basically the smallest font that could be used in a page and it rarely used. Paragraph text tends to be in 16px or 18px with a line-height of 1.5 for optimal readability. Your use of units seems really confusing to me too. You used the * selector to set the font-size to 12px, but you're using lengths like 7em to pad elements so that turns out to 84px which is a pretty odd way to size things. You end up with a whole lot of components that end up with fractional px lengths and you have no real harmony. You also set font size with em instead of rem which can be a real dangerous rabit-hole of compounding sizes. Since you're using sass you could make some mixins to consolidate on some styling rules that are repeated quite a but. You could also then make some partials like _variables.scss and _mixins.scss then import them into the large style sheet you have.

  I'd say overall the html looks nice, especially since you used the correct semantic element for an accordion. I think the styling could be tuned down quite a bit, there's quite a few cases of heights and widths you have set to a value that really don't need to be set at all. It seems to work well with the JS aspect, but imo I'd try to find a different method to create a responsive layout instead of just scaling the entire app container.

  2

  ---

  

  Adam•430

  @aemann2Posted almost 4 years ago

  @steventoben

  Thanks, Steven, for taking the time out to give such a thorough critique. I'm learning on my own so I'm finding a lot of gaps in my knowledge with projects like this.

  One big question I've always had is, when should I be using REM vs EM? I think I'm using them more less arbitrarily at this point, but only because I've never been clear on when to use which one.

  The reason I set the font-size to 12px for the root is because the style guide lists that the body copy is 12px. I guess that's why I assumed that the root font size should be 12px. I know 16 is normally the standard, but my question is, why is that something you would have to set at all? Wouldn't the browser just default to 16px? Or do different browsers have different root font sizes, and that's why people explicitly set the root font size?

  The reason I made the body a flex container was to center the main card. Perhaps I'm leaning too much on using flex to center things...I suppose I need to get to know the other ways of centering and decide if I should use those.

  1

  

  Steven Toben•750

  @steventobenPosted almost 4 years ago

  @aemann2 I totally get it, I was self taught as well. I started using React 2-3 years ago and had no clue what I was doing lol but time fills in the gaps.

  Rem vs em, the classic debate. I've seen so many blog posts around this topic and so much misinformation. So the two units are similar, but with a minor difference that can have major side effects.

  1rem = 1(base font-size). So modern browsers have a setting to change the overall font-size for all web pages. This is hardly ever changed by most users, and the default size is 16px. And to answer your question, I believe that you're correct. If you don't set the font-size in your html it should inherit from the web browser's settings. However there's always edge cases so applying font-size=100% covers the edge cases for the cost of a line, so I throw it in just in case someone out there's using IE7 or something lol.

  The em unit is a similar concept, but it's relativity changes. So the rem unit is relative to the root font-size, therefore it realistically won't ever change during the lifecycle of your application. The em unit is relative to the current selector's font-size (or the next highest selector which it can inherit a font-size value from). So this basically makes the em unit a multiplier. So if you have a text element and you set it's font-size to 20px. That means 1em=20px. If you had some element as a child of that text element and set it's height to 2em, then the child would inherit 20px as the em value and it's height would be 40px.

  The use cases are often debated, personally I use rem for setting font-size, so if I wanted something to be a font-size of 32px id set it to 2rem. Setting font-sizes with em can get out of hand FAST, since it will basically be multiplying and the value of em will increase at each rule. I also use rem for padding and margin, although em can sometimes in rare cases be a better fit. I often set heights or widths in rem if I need to, I try to avoid setting height and widths if I can. I think that's about it, for rem. I use em for media queries instead of using pixels, I also use em for letter-spacing. I set my line-heights with a unitless number, also I use rem for border-radius, and pretty much any other measurement I need. I think the most important one is to set your font-sizes in rem units.

  And yeah that makes sense, honestly the style guides are pretty weird to me, I don't find them to be a good representation of actual sizes. I think he might mean the body size, as in paragraph text size, which is often called body text, but even then 12px is too small to ever be used on a website really. After a long time of planning and designing I've come up with a typography system that I use for everything, and 12px is the smallest font-size and is rarely used. Body text is generally 14-16px on average. I feel as though the style-guide that's provided just causes confusion, especially to people who have no experience and are trying to learn by doing. I think the break points he gives in the style sheet confuse a lot of people. I was thinking about recording myself doing a simple challenge on here and explaining every line of code that I write and the importance of each thing I do, but I'm not sure how people would react to that. I notice a lot of people have tons of mistakes and are just trying to match the picture pixel for pixel, and they're not truly understanding how it should be solved.

  And for centering something flexbox is definitely super common, there are other ways of centering as well, but using it for that purpose is valid. The problem comes when every single element in the DOM is a div element that is a flex container. People really like to abuse flex. There's lots of other really good display types that can achieve the same goal.

  1

  

  Adam•430

  @aemann2Posted almost 4 years ago

  @steventoben

  Wow, really great summary of REM vs. EM. I knew that REM was set at the root, but I've never really heard anyone describe their rationale behind when they use which unit. I think just remembering to use REM to set font-size will take me a long way for now.

  And yeah, I did find the break points confusing. There's no happy medium between the given desktop and mobile layout dimensions, which is why I chose to scale the entire container (I'm going to redo that when I go back over the project...I mostly did it out of laziness).

  0
• 

  Jane•1,040

  @janegcaPosted almost 4 years ago

  Hi Adam, I think that if you add background-repeat: no-repeat and overflow: hidden to your body selector things will look better; although you might want to add a max-height on the mobile version because with the other adjustments the page can be scrolled off the screen. Also suggest you add an alt="" to your images, that will clear up the HTML and Accessibility errors. The images are only decoration so actual text descriptions aren't wholly necessary.

  Otherwise it looks pretty good. Happy coding.

  1

  ---

  

  Adam•430

  @aemann2Posted almost 4 years ago

  @janegca

  background-repeat: no-repeat sort of worked, but I ended up having a white line at the bottom of the screen instead of a colored one. after some hacking and looking at a comment above, the "fix" turned into me resetting margins and messing with min-height in the media queries. It looks close to how it should, but in looking over my code, my whole solution feels kind of hacky now. I think I'm going to come back to this in a few weeks and start a new branch where I overhaul the whole thing from scratch.

  But thanks for the compliment on how it looks! I am happy with that part, I just need to scale way back on some of the styles because right now it's a mess to manage.

  0
• 

  Nsikak•35

  @Nsikak1Posted almost 4 years ago

  Add a background-repeat: no realest; to the body as the background automatically repeats itself

  1

  ---

  

  Adam•430

  @aemann2Posted almost 4 years ago

  @Nsikak1

  didn't quite work, i actually needed to reset the margin. thx though

  0
• 

  T

  Grace•31,270

  @grace-snowPosted almost 4 years ago

  I'm afraid the font is too small for me to read this on mobile. Functionally it all seems to work, just really tiny

  1