First project with javascript v1.1

Community feedback

• 

  Joran Minjon•610

  @DrKlonkPosted over 3 years ago

  Hi Jeremy,

  Decent job! The carousel works and indeed the cat is very cute. I noticed the following things:

  Semantically calling the image block and the text block a separate section is something I would not do. It feels a bit like overkill. The text-block section will only ever have 1 article in this context, right?

  In your scss mixin you calculate px back to rem. I don't think this is necessary. Why not use rem directly?

  I also think this line for something basic like font-size is just too complicated. "font-size: clamp(0.9375rem, 0.9375rem + ((100vw - 375px) * ((20 - 15) / (1440 - 375))), 1.25rem);". Whats wrong with rem size based on media queries?

  Thanks for introducing me to some features with your code. I hadn't seen clamp before, for instance.

  To keep your footer at the bottom, it needs a parent element with its position set. Set the body's position to relative, for instance. You can replicate the behaviour I'm seeing by going into your devtools, fire up responsive mode, scrolling down, and then resizing the window height. The footer now flows on top of the content.

  Cheers! Joran

  2

  ---

  

  Jeremy Ng•200

  @lanechangerPosted over 3 years ago

  @DrKlonk Hi Doc! Thanks for taking the time to go through my solution and for the feedback :)

  Anything that you recommend using instead of section? I didn't use article as I read that articles should be able to standalone as their own content which maybe the text block does but the image doesn't. Or maybe I should have just left them as divs?

  Haha so for all my early solutions I did use just rems which was easier for me but got a feedback that it wasn't very readable so I decided to switch it up this time so the pixels values are easier to read and update but then they'll still be evaluated as rems.

  Yeah this font-size formula is also something new I've been cooking up that I need to think about how to make it more readable and self-documenting. So funny enough, I used to do media queries for font size in my previous solutions and again got feedback that it was too much code and too verbose so this is my succinct one line solution. It covers both the minimum and the maximum that are shown in the design sketches and then the calculation itself will scale the font size in a fluid fashion as you resize the window.

  Oh crap, nice catch on the footer! No one gave feedback on my footers in the past as they were working fine, this one I just broke myself XD I'll have to fix that up.

  Thanks again!

  0

  

  Joran Minjon•610

  @DrKlonkPosted over 3 years ago

  @lanechanger

  On the semantics: The thing with the section and article is that now, the image and text are semantically separated, while they are heavily related. I would probably not use a <section>. Apparently sections are pretty much supposed to have headers, for instance (https://developer.mozilla.org/en-US/docs/Web/HTML/Element/section). The <article> for the text is not so apt. You could use a <blockqoute> around the quote instead of <p>. It is a quote, after all. You can put a <footer> inside this blockquote with the person information (I also don't think the name and function should be in a <p> since it is not a paragraph). I think the text cannot be taken to another site, reused and still be selfexplanatory. But there is definitely a case to be made for the opposite.

  Mind you, I'm not great at this and am learning this as well. I have been googling a lot while writing this. The hard thing about this, is that there aren't very clear definitions. While somethings are clearly wrong and some are clearly good, there is a decent size grey area in the middle in my opinion.

  On the rems: Rems are just way more flexible and responsive by nature. Look up some articles on it, I'm sure they'll advice to use rems.

  On the font-size: well, more code isn't necessarily bad. I found this just an unreadable line; it is hard for me to figure out what is going on. Too much magic for just a font-size. But this is a matter of opinion, I guess.

  On the footer: I struggled with it a little bit myself when I wanted to make it behave the way I wanted. Got some nice advice on it here: https://frontendmentor.slack.com/archives/CDAPM34JX/p1614933779278700

  0
• 

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted over 3 years ago

  Hi, again, Jeremy Ng! 👋

  It's good to see another challenge from you! 😀 Good effort on this one! 👏 The extra slide you added is quite funny! 😂

  I was going to give you a quick rundown on how to create a simple image slider (since your JS looked a little more complicated than it needed to be, I think), but I got carried away and wrote a short blog article on it! 😆

  Here's the article (I created it after seeing your request for a resource for creating a slider)! 🙂

  Let me know if it helps! 😊

  Keep coding (and happy coding, too)! 😁

  2

  ---

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted over 3 years ago

  @ApplePieGiraffe

  And BTW, there's quite a bit of overflow to the right side of the page for the first two slides. I suggest adding overflow-x: hidden to the <body> (or something similar) to get rid of that.

  I also noticed that the outlines around the slider buttons look a little odd and the inspector shows some pretty complex calculations for the width and height properties of the buttons. I think things will be much easier if you simply set a fixed height and width for the buttons and scaled the SVG inside the buttons upon hover (rather than the entire button itself) so that the outlines don't look strange and you don't have to add overflow: hidden to the wrapper that contains the buttons.

  1

  

  Jeremy Ng•200

  @lanechangerPosted over 3 years ago

  @ApplePieGiraffe Hiya APG, thanks for checking out my solutions again!

  And thanks for writing an article about making a carousel, I will check it out and see how I can integrate it into my solution!

  Could you elaborate on the overflow, I'm not seeing what you mean.

  Yeah I didn't love how I handled the buttons, I'll re-visit it by scaling the svg, thanks :)

  1

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted over 3 years ago

  @lanechanger

  Hey, no problem! 👍

  I wish I could show you a screenshot of your page on my desktop screen because there's a bunch of extra space on the right side of the page (I think it's because of the way you positioned the inactive slides—they aren't visible, but they still take up space on my screen for some reason). Adding overflow: hidden to the body of the page should be a quick and easy fix. 😉

  For the buttons, I think you've added overflow: hidden to the container that wraps the buttons, meaning the outlines of the buttons look a little cut off at the corners and on the sides when they are visible. You shouldn't need to add overflow: hidden to the button container if you apply the border-radius to the buttons themselves, however, so you might want to consider doing that instead. 🙂

  Hope that makes it a little clearer. 😀

  1

  

  Jeremy Ng•200

  @lanechangerPosted over 3 years ago

  @ApplePieGiraffe Omg APG I literally dismantled my script and animation, tried out yours from the article you wrote, and then eventually redid what I had before with the overflow: hidden on main to prevent the extra spacing out on the side.

  There's probably more that I can refactor from here and I know there's an issue where if the viewport is small enough the active image will scale down but will snap back to its original size when it slides out but I'm happy with what I ended up with and will leave it for now to move on to other projects. Thanks again! :)

  1

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted over 3 years ago

  @lanechanger

  👍

  0