Skip to content
OverviewSolutions
8
Comments
14
Hossam khalaf
@hossam-khalaf

All comments

  • Hossam khalafβ€’230
    @hossam-khalafPosted about 2 years ago

    Thanks, @vcarames for Your Valuable feedback

    Ready To Build Your Community" is already an H3 heading.

    going to work on them right away:)

    Have a great day

    0
  • Hossam khalafβ€’230
    @hossam-khalafPosted about 2 years ago

    Hello, @abdelmounime

    Great Effort what you did so far.

    • You can use Semantic Elements like main tag instead of div you can read more about it here: Semantics

    • Also, you can add more padding-bottom or margin-bottom to the p tag to match the design

    Have a great day, Happy Coding😊

    Marked as helpful
    0
  • Rachel Johnsonβ€’510
    @rachelktyjohnsonSubmitted about 2 years ago

    I've never done something like this for the background so this is my first time. Is this the best practice for something like this? Thank you!

    Profile card component

    2
    Hossam khalafβ€’230
    @hossam-khalafPosted about 2 years ago

    hello, @rachelktyjohnson

    Great job what you did with the background-position.

    the only note I can give you about it is to use relative units instead of px

    your code is like that : background-position: top -600px left -750px, bottom -600px right -750px;

    you can change it to something like that:

    background-position: left 50vw top 40vh, right 46vw bottom 50vh; and you won't need to add any media-query for the background because the background position is relative now to the viewport width and height

    Have a great day, Happy Coding 😊

    Marked as helpful
    2
  • Israel Solomonβ€’20
    @Soulz001Submitted about 2 years ago

    This project was done using HTML and CSS. I'll appreciate review especially when it comes to the naming convention and the accessibility of the project.

    Main Card Component

    2
    Hossam khalafβ€’230
    @hossam-khalafPosted about 2 years ago

    Hello, @Soulz001 Great Job on what you achieved in the design so far.

    here are some notes:

    1- You can use the main tag as your container instead of adding non-necessary divs, your code will be more readable and much cleaner, also try to use semantic elements more, like <section> <articles>

    2- the mobile version of your projects has a scroll and doesn't match the design, after reading your code I saw that the padding on the pics__section causing that problem, it's so large for the mobile screen, decrease it to 9 or 10 rem or try using <picture> tag instead of using the background property on the <div>. -- you can read more about it here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture

    3- good job on background effect by the way but, You can use [mix-blend-mode] to make this color blend between the image and the background-color of the container. And play with the opacity to reach the actual effect.

    4- try increasing the font size of the heading to match the design and also adding some padding-right to text__section to match the desktop design, also don't forget to change it in the media-query -- which you can change to 600px for example instead of 1024 --

    don't get overwhelmed by all of the notes, there is nothing wrong with your code, it's just a better way of doing things, we are all still learning and we will always have more to learn about.

    have a great day Happy Coding😊

    Marked as helpful
    0
  • Hossam khalafβ€’230
    @hossam-khalafPosted about 2 years ago

    Hi, Remoan Great job you did with this project,

    You can use [mix-blend-mode] to make this color blend between the image and the background-color of the container. And play with the opacity to reach to the actual effect.

    Also you can increase the font size of the headings to match the design. Have a great day, Happy coding πŸ˜ŠπŸ‘Œ

    Marked as helpful
    1
  • Hossam khalafβ€’230
    @hossam-khalafPosted about 2 years ago

    Good Job With The Design Edgar! I see a little Problem with the Mobile Nav, The Bars icon doesn't disappear when clicking on it and it shows alongside with Times icon. The quick fix for that is to use conditional rendering with (useState hook) on the icon itself.

    also, you can use (position:absolute) on the mobile nav to match the design.

    I like the animation so much though.

    Good Luck :)

    0
  • Hossam khalafβ€’230
    @hossam-khalafPosted about 2 years ago

    Good Job Priyanshu Mishra congrats on completing your first challenge. a little modification: You can add a box shadow to your card.

    Marked as helpful
    1
  • Hossam khalafβ€’230
    @hossam-khalafPosted about 2 years ago

    Hello, Abdul Basit

    I think Your repository is empty check it again and push your project

    0
  • Hossam khalafβ€’230
    @hossam-khalafPosted about 2 years ago

    Thanks for your feedback Lucas, Maybe it's a habit from my previous work to check browser support of css properties because they were very strict about it πŸ˜‚ You can check it at mdn and caniuse.com. I used the trick of inset shadow, which is not long btw, just a single line. The result is Not 100% percent as the design, I know Anyway I like to see your feedbacks on my next challenges. Always helpful! Have a great day, happy coding.

    0
  • Hossam khalafβ€’230
    @hossam-khalafPosted about 2 years ago

    good job,

    try to use flexbox direction to achieve the mobile design with a media query.

    using semantic elements like "main", "section", "article" instead of using divs

    0
  • Hossam khalafβ€’230
    @hossam-khalafPosted about 2 years ago

    Good Job, it's great! I suggest using semantic elements like "main" for your container

    and "section" for the text section

    also, decrease the font size of your h1 to match the design

    good luck

    0
  • Mehdiβ€’990
    @siavhnzSubmitted about 2 years ago

    Hello dear community

    I've just completed this challenge with Tailwindcss.

    I started to create a mobile-first workflow by flexbox, and when I came to desktop design, I realized that using the Grid layout system was a better choice, so I continued with Grid for desktop.

    Any feedback will be appreciated.

    Product preview card component with Tailwindcss

    3
    Hossam khalafβ€’230
    @hossam-khalafPosted about 2 years ago

    That's awesome, identical to the design! about the design comparison: I think you need to generate a new screenshot to sync with the last update to your GitHub repo, if you are on a free plan like myself, you have limited screenshots every month.

    1