Skip to content
OverviewSolutions
10
Comments
37
Mo
@MohamedAridah

All comments

  • Mo•840
    @MohamedAridah

    Hello @inkfromblood, congratulation on submitting your first solution🎉👏.

    Yes giving the QR code image max-width: 100% is totally right since this will make the image to take the full width and never overflow it.

    I have also some notes for you:

    • use font-family: 'Outfit', sans-serif globally instead of redeclare every time:
    body {
    font-family: 'Outfit', sans-serif;
}

    • instead of using <h2> heading level for the text you can just use <p> element since this text is not a heading. This will fix your Accessibility Issues

    • use <footer> instead of .attribution div it's more HTML Semantic. Also This will fix your Accessibility Issues

    • This challenge Doesn't has nay links so you can Remove styles of the <a> tag since it's useless.

    • you can see My solution for this challenge it may be helpful for you..!

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @MorufLawal, congratulation on submitting your first solution🎉👏.

    I have some notes for you:

    • for the hover effect, put the image with .eqn class inside div with .image-wrapper class for example and follow these styles:
    .image-wrapper::after{
	content: url(./images/icon-view.svg);
	position: absolute;
	background-color: rgb(0 255 247 / 45%);
	width: 100%;
	height: 100%;
	opacity: 0;
	top: 0;
	left: 0;
	display: flex;
	align-items: center;
	justify-content: center;
	cursor: pointer;
	border-radius: 1rem;
	overflow: hidden;
	transition: .3s ease;
}

    .image-wrapper:hover::after {
    opacity : 1;
}

    i used div to wrap the image and do the hover effect since <img> alone doesn't work with ::after pseudo element.

    • instead of using <hr> tag you can use border-top or border-bottom depending on the div you will use it. Also this is more Semantic

    • for .Bottom give it text-align: start to be like the design also give space between the avater image and the text.

    • use <a> element instead of span for author name. It's more Semantic and on click this name this may lead to another page.

    • you can see My solution for this challenge it may be helpful for you..!

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

  • Mo•840
    @MohamedAridah

    Hello ,

    • for .parent you need to add background-color property to to add background for the element and make it like the design.

    • for .parent there is no need to use fixed height. height: 560px.

    • for .parent to make the waves illustration looks closer to the design you can change background-size property to 100% instead of cover. or you can remove background-size property at all. you cab read about background-size from Here.

    • So after all this changes to the .parent it's styles will be like this:

    .parent {
    display: grid;
    place-items: center;
    min-height: 100vh;
    background: #e0e8ff url(../images/pattern-background-desktop.svg) no-repeat;
}

    • for .container you can remove margin-top property if you want the card to be perfectly centered.

    • Don't use fixed height let content itself determine it's height. so you can remove height: 335px for .card-content div

    • for card-title instead of using <h2> heading level use <h1> heading level instead. This will fix your Accessibility issues as well.

    • when you use width: 80% instead of using margin-left or margin-right to center the content . You can just use margin: auto.

    • use font-family globally in the body for example instead of redeclare it for each element

    • .pay and .cancel have common styles so you can combine them in one class and add it to them instead of redeclare it again.

    • for your question you can see My solution for this challenge it may be helpful for you..!

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

  • Mo•840
    @MohamedAridah

    Hello @12Kentos, Good job it's almost identical to the design👍👏

    However i have some notes for you:

    • use min-height: 100vh for the body instead of using height: 100vh. This will allow your content to take more vertical height if content needed to. So user can see all the content.

    • instead of using these styles for .eth-cube-img

    .eth-cube-img {
    width: 30.02rem;
    height: 30.02rem;
    border-radius: 0.8rem;
}

    you can just give the <img> the full width and the padding of .eth-card-container will make sure that there are space around the image. So your styles for .eth-cube-img could be:

    .eth-cube-img {
    width: 100%;
    border-radius: 0.8rem;
}
    • for the hover effect you can Delete .eth-eye-img img tag and .eth-img-container styles and try pseudo elements like ::after or ::before like:
    .eth-img-container::after{
	content: url(./images/icon-view.svg);
	position: absolute;
	background-color: rgb(0 255 247 / 45%);
	width: 100%;
	height: 100%;
	opacity: 0;
	top: 0;
	left: 0;
	display: flex;
	align-items: center;
	justify-content: center;
	cursor: pointer;
	border-radius: 1rem;
	overflow: hidden;
	transition: .3s ease;
}

    .eth-img-container:hover::after {
    opacity : 1;
}

    • instead of using <p> element for .eth-title you can use <h1> because this is a a heading.

    • use <a> element instead of span for .eth-auth-name. It's more Semantic and on click this name this may lead to another page.

    • you can see My solution for this challenge it may be helpful for you..!

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @tbrownlee, you did Great for this challenge, it's also Responsive so good job.👏👍

    • your styles to .main is extra and has no purpose. So Remove it.

    • using flex with the <img> is also can be Removed. the <img> element isn't parent for an any element so display: flex will do nothing.

    • instead of giving width property to the .text or the <img> you can just let them take 100% of the width and use padding property to control and give the outer space around the .container.

    • it's better to use rem units for font-size and somethings like that. em is relative to the font-size of its direct or nearest parent . rem is relative to the HTML (root) font-size. Read this Article for better understanding.

    • you can see My solution for this challenge it may be helpful for you..!

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    @tbrownlee I'm glad this was useful for you🌹👍. you can read more about pseudo elements from Here.

    • Yes, you can change the icon size by using background-size property.
    .interactive::after {
    background-size: 55px; /* you can use percentage % as well */
}

    You can read more about it from Here.

    Keep Coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @Kratos012, you did very Good job for this challenge it's also Responsive so Well done👏👍.

    however i have some notes for you:

    • Please Recheck your solution background colors and make it like the design.

    • To insure .heade will take the full width you can use:

    .header{
	grid-column: 1/-1;
}

    your current way is totally right

    • you should use <h1> heading level once in the page. So instead of using <h1> tag inside .subscribtion div you can use <h3>. because the main heading of this div is <h2> and you should use heading levels by orders like: using <h1> then <h2> then <h3> and so on. This is Useful for SEO

    • For p element inside .whyus div. Try use ul element (unordered list) will be much better and this is More Html Semantic

    	<ul>
		<li>Tutorials by industry experts  </li>
		<li>Peer & expert code review </li>
		<li>Coding exercises</li>
		<li>Access to our GitHub repos</li>
		<li>Community forum </li>
		<li>Flashcard decks </li>
		<li>ew videos every week</li>
	</ul>
    • Your CSS code is Good ,but your styles for <h1>, <h2>, <p> and <a> are set Globally . I've just wanted to draw your attention to this

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @tbrownlee, you did Good job. and Your solution is Also Responsive.👍👏 it seems quite small to me but its ok.

    I have some notes for you:

    • inside .interactive div instead of using .interactive-overlay and .interactive-icon. You can use pseudo element instead Your way is totally right but you can try the following:
    .interactive::after{
	content: url(./images/icon-view.svg);
	position: absolute;
	background-color: rgb(0 255 247 / 45%);
	width: 100%;
	height: 100%;
	opacity: 0;
	top: 0;
	left: 0;
	display: flex;
	align-items: center;
	justify-content: center;
	cursor: pointer;
	border-radius: 1rem;
	overflow: hidden;
	transition: .3s ease;
}

    .interactive:hover::after{
	opacity: 1;
}

    • instead of using <hr> tag you can use border-top or border-bottom depending on the div you will use it. Also this is more Semantic

    • use <a> element instead of span for author name. It's more Semantic and on click this name this may lead to another page.

    • you can see My solution for this challenge it may be helpful for you..!

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @justcoder42022, impressive.! your solution layout is very good and almost similar to the design.

    However i have some notes for you:

    • instead of using display property to control showing and hiding of the icon on the image on hover. You can use for example opacity. This will allow you to use transition property with it and hover will be much smoother. Since display doesn't work with transition.
    .main-image-section::after {
        left : 0; /* to indicate starting point horizontally   */
        opacity: 0; /* use opacity instead of display */
        transition : opacity 200ms linear;
    }

    .main-image-section:hover::after {
        opacity: 1; 
        cursor : pointer; /* better than the default and design did that too */
    }
    • For .main-image-section::after you can use background property shorthand like:
    .main-image-section::after {
        background: hsla(178, 100%, 50%, .6) url("images/icon-view.svg") no-repeat center;
    }
    • For .main-image-section::after instead of using height: calc(100% - 4px). You can just give the <img> display : block property and everything will work fine.
    .main-image-section::after {
       height: 100%;
    }

    .main-img {
       display: block;
    }

    • instead of using strong element inside .avtar-containter div. You can use <a> element. It's more **Semantic ** and on click this name this may lead to another page. Also don't forget to add transition for it on hover.

    • using:

    li {
  list-style: none;
}

    this way means you want to use it Globally. If so, it's better to put it at the beginning of the code. Optional and Your way is totally right

    • you can see My solution for this challenge it may be helpful for you..!

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @robinjmm,

    I have some notes for you:

    • use <a> element instead of span for .nft-card__creator-name. It's more Semantic and on click this name this may lead to another page.

    • you can just use <img> instead of .nft-card__eth::before and .nft-card__timer::before. the current way is totally right.

    • instead using both ::after and ::before pseudo elements you can use one of them as the styles below and everything will work fine:

    .nft-card__bg-layer::after {
        content: url(./images/icon-view.svg);
        position: absolute;
        background-color: rgb(0 255 247 / 45%);
        width: 100%;
        height: 100%;
        opacity: 0;
        top: 0;
        left: 0;
        display: flex;
        align-items: center;
        justify-content: center;
        cursor: pointer;
       border-radius: 1rem;
        overflow: hidden;
        transition: .3s ease;
	}

    .nft-card__bg-layer:hover::after {
        opacity: 1;
	}

    • use transition property for smoother hover effect for both .nft-card__heading and .nft-card__creator-name

    • you can see My solution for this challenge it may be helpful for you..!

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @milosshomy,

    I have some notes for you:

    • don't use height for .card this causes big space in the bottom of your card. let content inside the card take the suitable spaces like padding and margin instead.

    • when you remove height for the .card div you will need to add padding-bottom for the .card-content div

    • remove margin-bottom for the image. you can use it for .card-image div itself.

    For Accessibility issues:

    • wrap .card div inside <main> tag.

    • use <h1> heading level instead of using <h3> heading level.

    • you can see My solution for this challenge it may be helpful for you..!

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @khalilnazari, your solution is very impressive, and your code is clean enough.

    • the only note for you is to use min-height: 100vh for the body instead of using height: 100vh. This will allow your content to take more vertical height if content needed to. So user can see all the content.
    • you can set text-align directly to .card .card__body div instead of h1 and p. because they both share the same value. The current solution is totally fine and right

    For Accessibility issues

    you can just put .container div inside <main> tag. This should fix your accessibility issues.

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @sanketcharanpahadi, you solution is looking very good.👍👏

    I have some feedback for you:

    • you can run getQuote() function when page loads for getting new advice every time.
    window.addEventListener('load', getQuote);
    • you can use button instead of .expand div. It will be more Semantic.
    <button class="expand flex">
      <img src="images/icon-dice.svg" alt="">
</button>

    just add this property to the .expand

    .expand {
    all: unset
}

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @justcoder42022, you did good job for this challenge.

    However i have some notes:

    • change height: 100vh of the body to be min-height: 100vh. this will allow content to take more height of the body if it needs to.

    • use transition property for smoother hover effect.

    • use overflow: hidden for .wrapper. by doing this you don't need to set border-radius for the image.

    • there is extra space on the top and bottom of the main element. try to know it's source and remove it.

    • you can see My solution for this challenge it may be helpful for you..!

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @DefinitelyObsessed, your layout looks great ,but you need some improvements like:

    • use transition for smoother hover effect.
    • you have problem with the image border-radius is not applied correctly.
    • instead of using padding for each element separately. just give it to the .parent-block and it will works fine
    • I strongly recommend you to take a look for My solution for this challenge to see HTML structure and how to deal with the image.

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @DefinitelyObsessed, very good job there. your solution is almost identical to the design.

    However i have some notes for you:

    • There is other way to center .parent-card instead of position. like setting display: flex or even display: grid to the body element. Your way is totally right
    	body {
		display: flex;
		justify-content: center'
		align-items: center
	}

    	body {
		display: grid;
		place-items: center
	}
    • in the body styles you can use background property shortcut. like:
    	body {
		background: hsl(225,  100%,  94%) url("./images/pattern-background-desktop.svg") no-repeat;
	}
    • To be more HTML Semantic you can use <a> element for pay cancel order below. because this action may lead to an other page to continue the process.

    For Accessibility issues

    • use main element instead of .parent-card div.

    • change <h2> heading level to be <h1> heading level. this should fix your accessibility issues.

    • you can see My Solution for this challenge it may be helpful for you.

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello @DefinitelyObsessed , Great job for this challenge.

    I have some notes for you:

    • As LiBee mentioned above the card not centered. This problem cause horizontal overflow for small screens.

    • Add transition property for <button> elements on hover for smoother effect on hover on it

    • you can leave card's image alt attribute Empty. Because these icons not necessary it's just for decoration. And this one of the cases leaving alt attribute empty is a good practice.

    • instead of redeclaring font-family for each element individually. You can set it to the body globally.

    	body {
		font-family:  "Big Shoulders Display", sans-serif;
	}

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful
  • Mo•840
    @MohamedAridah

    Hello there, Richard! 👋. Good job.

    you can do simple improvements for this challenge:

    • instead of using border-radius separately for .sedans and .luxury divs and change them again by mediaquery. You can set border-radius just for the .container and everything will work fine.
    	.container {
		border-radius: 10px;
		overflow: hidden; /* to crop radius perfectly */
	}
    • For active state for button elements. you can change cursor of the mouse to pointer to inform user this is clickable
    	button:hover{
		cursor: pointer;
	}
    • To be more HTML Semantic you can use <footer> element instead of .attribution div.

    I hope this wasn't too long for you, hoping also it was useful😍.

    Goodbye and have a nice day.

    Keep coding🚀

    Marked as helpful