Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

Article Preview Component using HTML/CSS - Flexbox and a little JS

Josh Prenticeβ€’ 235

@jfprentice

Desktop design screenshot for the Article preview component coding challenge

This is a solution for...

  • HTML
  • CSS
  • JS
1newbie
View challenge

Design comparison


SolutionDesign

Solution retrospective


I am always open to feedback and appreciate any and all critiques!

This was the first project I've done that has used JS - I've been wanting start incorporating JS into my projects but I wasn't sure if I was good enough with HTML/CSS yet to branch off to JS. I also don't have much experience with JS DOM manipulation.

Even though the JS part was really simple, I did have a few issues getting it to work properly - tried using document.getElementsByClassName but couldn't get it to work properly so I had to add a couple of id tags and use .getElementById. I also had an issue toggling the visibility of my share div. I tried using a ternary conditional statement and then just an if/else, both would only make the div visible but not back to hidden. (I left my ternary conditional in the index.js file as a comment if someone wants to look and see where I went wrong) I ended up making a separate class for visibility and used .toggle() to switch between the classes.

As before, any tips/tricks/critiques are welcome - your feedback really helps me improve!

Thanks for looking!

Community feedback

Kerri-Annβ€’ 330

@Kerri-AnnBates

Posted

Hi, I think this looks great! Good job. I saw a comment in your js file about this line of code not working:

x.style.visibility ="hidden" ? "visible" : "hidden";

It probably did not work because your condition is assigning the visibility style to "hidden". I think what you're looking for is the equality operator. Using == instead of =

Like so: x.style.visibility =="hidden" ? "visible" : "hidden";

That may work. Hope that makes sense and that it helps :)

2

Josh Prenticeβ€’ 235

@jfprentice

Posted

Thanks for looking at my code! I definitely overlooked that I was using the assignment operator - I'll go back and see if that was the culprit! Thanks again!

1
Kerri-Annβ€’ 330

@Kerri-AnnBates

Posted

@jfprentice You're welcome! I actually read up on this myself after seeing your code and found out that, the solution I posted in my previous comment would not work either because x.style will only work to check inline styles and not styles from an external stylesheet.

After some research I came across this documentation: (https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle) which looks like is a more likely way to check if an element's css property has a specific value. Something else you can look at. I learned something new with this too!

1
Josh Prenticeβ€’ 235

@jfprentice

Posted

@Kerri-AnnBates I did go back and change the operator but with no result. That's good to know about .style though! I have a lot to learn when it comes to JS and DOM manipulation. Thanks for the link!

0
Roman Filenkoβ€’ 3,335

@rfilenko

Posted

Hey, good job on this one. To make a card be centered on a page you missed one important property - min-height. So set on a body {display: flex; justify-content: center; align-items: center; min-height: 100vh;}. Also wrap all other child elements inside one container, that should do the trickπŸ˜‰, also disable margins, paddings as well.

Cheers, Roman

1

Josh Prenticeβ€’ 235

@jfprentice

Posted

@rfilenko you are 100% correct - I'll need to go back and make a few changes to my share div positioning but that definitely centered it! Thank you!

0
P
ApplePieGiraffeβ€’ 30,545

@ApplePieGiraffe

Posted

Hey, Josh Prentice! πŸ‘‹

Nice job on this challenge! Your solution looks good, responds well, and your social media share button works great! πŸ™Œ

I only suggest,

  • Centering the card in the viewport at all screen sizes, either by using auto margins or flexbox or something. Currently, the large padding at the top of the page pushes the card off center at some screen sizes.

Keep coding (and happy coding, too)! 😁

1

Josh Prenticeβ€’ 235

@jfprentice

Posted

@ApplePieGiraffe that's what I was trying to achieve with the padding haha I thought I had it - I always have trouble getting things vertically centered. Using auto margins will get it horizontally centered but the top of the card is still at the top of the page. I tried applying flexbox to the body with justify-content and align-items as center but I ended up with the same result as the auto margins. I'm going to do a little more research and see how I can fix it. Thanks for checking out my work!

0

Please log in to post a comment

Log in with GitHub
Discord logo

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord