@d3monviking
Submitted
@dtp27
@d3monviking
Submitted
@dtp27
Posted
Hi Priyanshu!
That looks really good overall! The one thing I would recommend is to take care of the HTML and Accessibility issues. You want to make sure to include the alt=""
property, not just for accessibility, but also incase the image doesn't load for whatever reason.
Also, adding properties like aria-label
is a good idea for anchor tags to describe where the link would take you. Here's a good set of articles from MDN. Hope this helps.
Happy coding!
Dan
@cristinakellyt
Submitted
Hi, all! I thought this one would be easy, but it took a while to find a good and proper way to put the hover effect on the image. Anyway, it was a good challenge! All feedback and improvements are welcome =)
@dtp27
Posted
Hi Christina!
That looks fantastic! I noticed that you're using the BEM naming convention. It was recommended to me that I start using it as well. How have you liked using it and how does it compare with anything you've used in the past?
Thanks!
Dan
@dhruvjha206
Submitted
@dtp27
Posted
Hi @dhruvjha206!
Congrats on your first challenge, and welcome to Frontend Mentor! A few recommendations I have on your solution:
p
instead of span
so that it's treated like a block component in normal flow instead of an inline component.flex-direction: column;
to make sure that the card and footer are stacked vertically instead of horizontally.img
width, I would use max-width: 100%
; so that it responsively adjusts to screen size..container
to 1440px
, which is the total width of the page the design is at. Instead, I would play around with % for the width of the .container
, which will make the card responsively adjust.Let me know if you have any questions.
Happy coding!
Dan
@iTwiixZ
Submitted
@dtp27
Posted
Hi Alexis!
Pretty good solution overall! A few things I would recommend:
body
, since that will then center your entire card on the page. Also include min-height: 100vh;
in the body to ensure the content takes up the whole page (i.e. it'll be centered relative to the entire page).
2 . I would also change the width
property in your img
to max-width: 100%;
. This will ensure the image scales down responsively with smaller screen sizes.body
, then using rem
to scale fonts from the different elements up and down. This can make it easier to control all of the fonts.<br>
elements in your paragraph to match it "line-for-line". Pay attention to the design font sizing and weights, and it should take care of itself.Let me know if you have any questions or thoughts.
Happy coding!
Dan
Marked as helpful
@HammadEngr
Submitted
@dtp27
Posted
Hi Hammad!
Pretty good solution overall! Flexbox makes it super easy to center the NFT component on the page. You would to add something like this to your body:
body {
min-height: 100vh;
display: flex;
flex-direction: column;
justify-content: center;
align-items: center;
}
That will center the direct child elements on the page, which in this case is just main
(your component).
Let me know if you have any questions about this, thanks!
Dan
Marked as helpful
@fica25
Submitted
@dtp27
Posted
Hi Filip!
Great solution overall! Make sure you style your horizontal line in the component. If you're using HTML to make a <hr>
component, you can do something like this in the css:
hr {
border: 1px solid hsl(215, 32%, 27%);
}
Also, pay attention to the font-weight
property for the different elements to make sure they are similar to the design.
Happy coding!
Dan
Marked as helpful
@Engfathy
Submitted
@dtp27
Posted
Hi Fathy!
That looks really good! I would make sure your h1 is not too big - one of the best piece of advice I got was to use rems
for all of my font sizes when I can instead of px
. That made it much easier for me to control all of my font sizes relative to each other.
Also, what was the reasoning behind splitting out the css into three separate files?
Happy coding!
Dan
@prasannarames
Submitted
@dtp27
Posted
Hi Prasanna!
Congrats on your first challenge, and welcome to Frontend Mentor!
Great solution overall. One thing I would recommend to center the overall component on the page vertically is to use Flexbox on the body, along with min-height: 100vh;
. The minimum height is to ensure that when flexbox is enabled it is centered relative to the entire view.
Happy coding!
Dan
Marked as helpful
@evilboysameer
Submitted
@dtp27
Posted
Hi Sameer!
Congrats on your first solution, and welcome to Frontend Mentor!
Great first solution! A couple things I would recommend:
min-height: 100vh;
in your body CSS. This will have the body take up the entire view from the start and ensure that the background color takes up the entire view as well. It will also ensure that your component is centered on the page.Hope this helps. Happy coding!
Dan
@PGFM
Submitted
Feedbacks are welcome
@dtp27
Posted
Hi @PGFM!
Congrats on your first challenge, and welcome to Frontend Mentor!
Great solution overall! One thing I would pay attention to when comparing the design with the solution is the overall dimensions of the component. Try using percentages instead of pixels to set the width of .inner
and play around with getting the solution closer in size to the design. I think all of your text looks great but because the component is larger it looks a little too spaced out.
Happy coding!
Dan
Marked as helpful
@Yoseif-Alfiky-1
Submitted
All opinion is acceptable , Tell me what do you think i did it wrong to improve myself. Good luck for every body
@dtp27
Posted
Hi Yoseif!
Great solution! Another way to center your component, because you're actually really close with using Flexbox in the body, is to add min-height: 100vh;
. This will make sure the body takes up the entire viewing area, which will result in Flexbox centering the component in the view.
One thing I would caution about using fixed position is that it takes the element out of normal flow and can cause interesting and unintended side effects when dealing with more components on the page.
Happy coding!
Dan
Marked as helpful
@Robert0362
Submitted
@dtp27
Posted
Hi Robert!
Congrats on your first challenge, and welcome the Frontend Mentor!
Great first solution! One thing I would recommend is setting the min height in the body element to ensure that the background color takes up the entire viewing area and also the component gets centered properly. You can then also use flexbox on the body to center the entire component:
body {
min-height: 100vh;
display: flex;
justify-content: center;
align-items: center;
}
Hope this helps. Happy coding!
Dan
Marked as helpful