Skip to content
OverviewSolutions
5
Comments
6

Osama Elshimy

@Osama-Elshimy140 points

I’m a mysterious individual who has yet to fill out my bio. One thing’s for certain: I love writing front-end code!

Latest solutions

Latest comments

  • Osama Elshimy•140
    @Osama-ElshimyPosted over 2 years ago

    Nice work!

    Here is how you can toggle between the two images:

    • Add both images to the HTML file:
    <img
	class="desktop-img"
	src="images/image-product-desktop.jpg"
	alt="Product image" />
<img
	class="mobile-img"
	src="images/image-product-mobile.jpg"
	alt="Product image" />

    In CSS:

    .mobile-img {
    display: none; // Completely hides the image
}
    • Inside the media queries:
    (max-width: 500px)
{
    .desktop-img {
          display: none; // Hides desktop image
}

    .mobile- img {
          display: block; // Displays mobile image
}
}

    A few points you can improve:

    • Every a element and img element should have an alt attribute that describes the image and what the link does. This is essential for screenreaders. You can add the alt attribute like this: alt="YOUR TEXT"

    I hope you find this helpful.

    Happy Coding!

    Marked as helpful
    1
  • Osama Elshimy•140
    @Osama-ElshimyPosted over 2 years ago

    Nice work.

    Here are some points you can improve:

    • You should change the background color of the selected rate.

    • You can add animation on the input and button elements like this:

    .number, .button {
    	transition: all 0.5s;
}

    Read more about transitions here https://www.w3schools.com/css/css3_transitions.asp

    • I can actually go submit the form without choosing a rate. This is a problem. The user shouldn't be able to proceed unless they choose a rating.

    You can fix it like this:

    // Insdie the submitHandler function. Add the following line right after the loob
if (!selectedValue) {
    return; // If the user doesn't select a rate
}

    Overall, that was a nice job. I hope you find this helpful.

    Happy Coding!

    Marked as helpful
    0
  • Osama Elshimy•140
    @Osama-ElshimyPosted over 2 years ago

    Great work and very clean code!

    Here are some points you can improve:

    • Every image should have an alt attribute. This is essential for screenreaders. You can set the alt attribute like this:
    a="A QR code to FrontendMentor website"

    • You should replace the div element with the .card class with a main element instead. div elements are generic elements. They don't have a semantic meaning and you should put your main content inside of a main element.

    • The div elements with the classes of (text, details) are not necessary, you can remove them and add the text class to the h3 element and the details class to the p element

    I hope you you find this helpful

    Happy Coding!

    Marked as helpful
    0
  • Osama Elshimy•140
    @Osama-ElshimyPosted over 2 years ago

    Nice work!

    Here are how I did the JS event listeners:

    • Select the parent element of the rating buttons like you did my friend:
    const ratingList = document.querySelector(".rating-group"");
    • Select all the rating buttons together:
    // This will choose all the button elements and give you a Node List 
const ratingBtns = document.querySelectorAll(".rating-btn");
    • Select the span element with the rating-value class:
    const selectedRate = document.querySelector(".rating-value");
    • Select the form:
    const form = doqument.getElementById('form');

    // Select rating handler
//  1. Add event listener to common parent element - (ratingList)
ratingList.addEventListener("click", function (e) {
	e.preventDefault();

	//  2. Determine what element caused the event - Very important step
	const clicked = e.target.closest(".rating-btn");

	// Gaurd Clause - return if no button is clicked
	if (!clicked) {
              return;
        }
	// console.log(clicked);

	// Remove active class from all buttons
	ratingBtns.forEach(btn => btn.classList.remove("active"));

	// Add active class to clicked button
	clicked.classList.add("active");

	// Assign rate to clicked button value
	selectedRate.textContent = ` ${clicked.textContent}`;
});


// Submit form
form.addEventListener("submit", function (e) {
	e.preventDefault();

	// Make sure a rating is selected
	if (!selectedRate.textContent) {
		console.log(`You must Choose a rate!`);
		return;
	}

	console.log("Submitted");

	// Hide rating container
	ratingContainer.classList.add("hidden");

	// Display thank container
	thankContainer.classList.remove("hidden");
});

    The rating part is possible because of something called event propagation. You can read more about that here:

    [Event Propagation] (https://www.tutorialrepublic.com/javascript-tutorial/javascript-event-propagation.php)

    Overall that was a great job on the UI, you just need to refactor the JS code.

    If you find this helpful, click on the Mark as helpful button.

    Happy Coding

    Marked as helpful
    0
  • Osama Elshimy•140
    @Osama-ElshimyPosted over 2 years ago

    Nice work!

    A few points to improve:

    In HTML:

    • Every a element should have an href attribute. If the link doesn't go anywhere, assign it to # href="#"
         <div class="cancel-order">
             <a href="" class="cancel">Cancel Order</a>
     </div>

    • I also saw that you left an image without an alt attribute. alt attributes are essential for screenreaders. The alt attribute should be descriptive of the image.

    • You are using div elements where you don't have to, this is a bad practice. Only use div elements when you need to wrap other elements inside one container.

    • You didn't need the div element with the card class. You can remove it and add the class to the main element.

    • The div element with classes of content, order, button are not necessary.

    In CSS:

    • Never never use position: absolute and position: relative unless there is no other solution. Flexbox and Grid are much easier and much more powerful.

    • You're using position: absolute and position: relative where you don't have to. You can instead use flexbox to align and center the elements.

    • Flexbox is more than enough in one-dimensional layouts, so you didn't need to use Grid in this challenge. Grid is awesome of course, but it was not necessary.

    • You declared the universal selector * twice. You can add all the properties once like that:

    * {
    box-sizing: border-box;
    margin: 0;
    padding: 0;  // It's a good practice to set the `padding` to 0
}
    • In general, you shouldn't depend on element selectors too much. Use classes and ids instead.

    I know that was too much to take in my friend. You did a great job and the learning never stops. If you need any more help, feel free to tell me in the reply.

    If you found this helpful, click the 'Mark as helpful' button.

    Happy Coding

    1
  • Osama Elshimy•140
    @Osama-ElshimyPosted over 2 years ago

    Nice Work!

    You asked for a way to implement the select rate part. Here is how I did it:

    • Give the span element an id or a class like you did, you gave it an id='rate'
    • Select the element in JavaScript:

    const rate = doqument.getElementById('rate')

    • Select all the button elements like you did my friend:

    const btns = doqument.querySelectorAll('.btn')

    • Add event listener to a common parent element btn-list
    document.querySelector('.btn-list').addEventListener('click', function(e)) {
// Determine what element caused the event - what button was clicked
const clicked = e.target.closest(".btn");

// return if no button is clicked - if the user clicks on the list button but NOT on a button 
if (!clicked) {
    return
}

// Assign the value of the clicked button to the span element
rate.textContent = clicked.textContent // No space 

// You can add space before the number by using bracket notations like this:
   rate.textContent = ` ${clicked.textContent}` // There is a space
}
}
    • Final step: When you click the submit button. You have to make sure that the user chooses a rating:
    // return if NO rating was chosen - No button is clicked
// If no rating was selected, then the value will be null
if (!rate.textContent) {
   return
}

    The UI is very nice, but you must improve the semantic HTML. Use form instead of ul

    If you need more help on the JavaScript part, feel free to reply and I'll be glad to help.

    Happy coding :)

    0