P
@jguleserian
Joshua Hellard
@jhellardAll comments
- @jhellard
Hey Jeff! The solution looks great, I like the drop-down (or drop-up? 😂) menu a lot.
The CSS is very well structured, I will say the selector approach used is not one I see often. I gravitate towards the Block, Element, Modifier (BEM) approach myself, It's a more concise way of writing class names that I believe makes it easier to maintain in the long run, especially using something like Sass for nested selectors. Native CSS nesting is a thing now but it still very early, I'd take a look at THIS video for more information.
HTML structure looks good, great use of semantic tags. There are a few containers that aren't necessary but nothing excessive.
If you are not opposed to importing libraries, there is a great one called Moment.js that I often use for getting dates formatted, it is super simple to work with.
As for the CSS hover question, you could change the 'fill' on the SVG to 'currentColor' and when you modify the 'color' in CSS it will change accordingly. You could then use a custom CSS class to modify the div and the svg on hover to the desired colors. Here's an ARTICLE more about this. I'd be happy to throw a CodePen together for you to demonstrate this if you'd like, I know it can be a bit confusing.
Keep up the great work! Happy coding!
Marked as helpful - @imtiazraqib@jhellard
Hey Imtiaz! Storing the prop data in variables is a good idea, for this app it's not a big deal but if you were rending multiple cards you could have an array of data and map over it to easily pass the different values to the component.
Great solution, happy coding!
- @marisfreire@jhellard
Hi Mariana, great solution!
You can use a img element and pass in the svg as the src, that would allow you to set an alt tag for your images. Utilizing this approach you could change your container-image from a div to an img and just apply the same CSS.
Here's an article that describes this in more detail. Happy coding!
Marked as helpful - @OnealSpain@jhellard
Hi Wilson! It looks like your images aren't loading properly, if you can get that fixed I'd love to give some feedback!
- @ProwlingLynx@jhellard
Generally most people do a 'Mobile First' approach, styling out the page for mobile viewports and then adding breakpoints when scaling up as needed. Some frameworks have preset breakpoints you could look at to get some ideas of what to use.
Not sure if it's the best practice but I'll generally get my full page looking good and committing that CSS, with a commit message of like 'Mobile design completed'. Of course, you never catch everything the first time so I'll add minor updates to design with future commits as a side-note.
Testing CSS can be tedious, you can edit the CSS properties in dev tools and get it right then change it in your editor once you get your desired effect.
I hope I could provide some insight :) Keep on coding!
Marked as helpful - @Strocs@jhellard
Looking good! I'd suggest refactoring your handleCounter function to check if the message is read, it will continue to run even when the message is marked as read. Good use of useState, this is a good challenge for learning React for sure!
A fun challenge for this one I'd say is to make your data rendered dynamic, maybe pulling in from a .json file and mapping over it with your Notification component.
Keep on coding! ✌️
Marked as helpful - @kayegalon@jhellard
Looking good NIÑA! For your
img-dashboardclass I'd suggest not setting a fixed size, allowing images to adapt to the screen size and still look great can be one of the trickiest things to do. Try something like :width: min(100% - 2rem, 800px);You can read more about the
min()function here but what it's doing in that example is setting its minimum width to 100% of the parent - 2rem (Gets averaged to 1rem each side) and the 800px is setting the maximum width it will grow to. Give it a go, it's a super handy function! - @Yavanha@jhellard
Hey y4rb0w! Your solution looks great! The HTML looks good, great use of the semantic tags. Everything looks super close to the design, the lightbox functions very well. I love the animated shopping cart, really nice touch. I did notice when scaling down the screen size the header starts to overlap before hitting the mobile breakpoint, I'd take a look at that.
The
carousel-thumbnailsdiv wraps before hitting that breakpoint causing one of those images to take full width.Awesome work, keep on coding!
Marked as helpful - @Geek-cloud1@jhellard
Hey Joshua! For something with a simple layout like the QR code if you find there is no need for the breakpoint I'd just go without it as long as the content is displayed properly at all screen sizes.
Marked as helpful - @Hibiscuit0@jhellard
Assuming your container is 100% of the height/width you can use
display: gridandplace-items: centerto center is all very easily. The method you used is not bad however, there is just better ways now.Marked as helpful - @idowuadekale@jhellard
I can't get the website to work on my end, when I load the page I get 'An error occurred! Input correct IPv4 or IPv5 address or check your internet connection.'
I believe it's meant to say IPv4/6, possibly a typo?
If you can get it fixed I'd love to take another look but right now it's not functioning properly.
Marked as helpful - @Duck322@jhellard
You can use either @import or simply have a link tag in your index, both work the same. I will say I don't generally see people using @import in their CSS files, I'd recommend placing them solely in your index file.
Marked as helpful - @Ghravitee@jhellard
The overlay method is a simple way to get the effect, it's even the recommended method on W3Schools so there is no harm in it. I'd say the cleanest way is probably using either the
:beforeor:afterpseudo-element to avoid having another element.Here's a handy article on how to achieve that from 30secondsofcode.
- @eneyedev@jhellard
Looking pretty good!
I noticed a couple things, I was unable to close the hamburger menu after opening it and the "Creations" section has a lot of spacing compared to the design. I see you used the
gapproperty so maybe lowering it a bit would help. The site feels a little cramped with the breakpoint so high, I'd suggest moving it down to a lower width.Other than that, it looks great. Good job, keep coding!
Marked as helpful - @csubiii@jhellard
Good job on this solution! Your text-container does not need flex, simply leaving the
text-align: centeris all you'd need. You could even go without the text-container all together and just place your H3 and P tags in the main container, adjusting your CSS declarations for elements.Marked as helpful - @CrypticMango@jhellard
Looks pretty good! The border-radius isn't working properly on desktop, you could also just use one image and change the source dynamically in your CSS. Here's an article about that - https://simplernerd.com/css-img-src/
ID's are generally only used when targeting elements with JS, using class is recommended instead when just modifying with CSS.
Marked as helpful - @jhaynes322@jhellard
All you'd need to do to center it here is set your min-height to 100vh, take out your padding and un-comment your align-items on the container class.
Looking good on desktop, make sure to account for the mobile view however!
Marked as helpful - @hlainehtet@jhellard
Looking good! 😀 I feel the containers could be simplified, a flex container centering everything should be all you'd need. Then you could just make the card a flex container and style from there. Make sure to update your README.md so others can understand your thought process when putting this together, other than that it scales well and looks just like the design.
Good job, keep on coding! ✌️
