@fargada2356Submitted 18 days ago
Grego
@Grego14All comments
- @Grego14Posted 18 days ago
Hello!
The image doesn't load because you don't have the
assets/imagesfolder in your GitHub repo.You must upload the image to the GitHub repository and update the links.
I recommend using GitHub Pages or Netlify to host the website and thus avoid tiiny.site adding that footer.
Marked as helpful1 - P@amancalledkiddSubmitted about 1 month agoWhat are you most proud of, and what would you do differently next time?
I am most proud of using the picture element to responsively change the image depending on page size. This was the first time I have used it, it seems a more efficent way of dealing with responsive pages that require multiple images.
<picture> <source srcset="./images/image-product-desktop.jpg" media="(min-width:610px)" /> <source srcset="./images/image-product-mobile.jpg" /> <img src="./images/image-product-mobile.jpg" alt="Product image" /> </picture>Next time I will give better class names to my code.
What challenges did you encounter, and how did you overcome them?I found getting the images to fit the container to be the most difficult. Eventually I found object-fit which helped solved the issue. Will need to look more into styling images in the future
What specific areas of your project would you like help with?Did I use the picture element correctly?
Also is the scss structure correct?
Any suggestions on improving code?
@Grego14Posted about 1 month agoHello! Congratulations on completing the challenge. 🎉
You made a correct use of the picture element and I wanted to tell you that it is not necessary to place the
<source>of the image that you place as fallback because if no<source>complies with the media-query the fallback will be used.This example will do the same as yours:
<picture> <source srcset="./images/image-product-desktop.jpg" media="(min-width:610px)" /> <img src="./images/image-product-mobile.jpg" alt="Product image" /> </picture>To remove the space created below the image you can use vertical-align: middle; on the
<img>element.I suggest you use min-height: 100vh; Instead of height: 100vh; since on mobile devices the content is cut off, and it's impossible to see the full button.
I hope this helps! 😁
Marked as helpful0 - @AhmedHamada12354Submitted about 1 month ago@Grego14Posted about 1 month ago
Hello! Congratulations on completing the challenge. 🎉
I saw that you are using
<br>tags to create line breaks, that is considered a bad practice, and you should use CSS to do what you want to achieve there.I realized you're adding a not class, and you don't use it at all.
It's hard to see the results on mobile devices, I think you should decrease the size of the elements or try the mobile first approach.
Hide non-semantic images or icons from screen readers using the aria-hidden attribute with the true value.
I hope this helps! 😁
0 - @Abdelrahman-AlmansorySubmitted about 1 month agoWhat are you most proud of, and what would you do differently next time?
First time looping through classes using getQuerySelectorAll, its still difficult but i managed to pull it. I like the end result and waiting for feedback.
What challenges did you encounter, and how did you overcome them?removing minus class for buttons of a section when another sections open
@Grego14Posted about 1 month agoHello! Congratulations on completing the challenge. 🎉
I recommend using buttons instead of a
<div>for the icon elements and clicking on the text next to the icon also expands the accordion, as this would improve the user experience.I see that you're using multiple loops to do things that one would do. You can add an ID to the parent element of all accordions, the accordions-section, then get that element using:
const accordionParent = document.getElementById('accordion-parent-id')Doing this would no longer require the icons or the text, as you could easily obtain it using the accordionParent variable. And you can use the event delegation technique to improve your code:
accordionParent.addEventListener('click', (event) => { const target = event.target // if the clicked element is not an icon or a accordion text do nothing if(!target.classList.contains('icon') || !target.classList.contains('accordion-header')) return })To avoid using a loop and removing the minus class from each element, you can simply get the current element that contains the class, remove it, and add it to the currently clicked element:
// inside the accordionParent click event after the **if** // get last open accordion and accordion text const lastOpenAccordion = accordionParent.querySelector('.accordion:has(.minus)') const lastOpenAccordionText = lastOpenAccordion.querySelector('.accordion-content') // get next open accordion and accordion text const clickedAccordion = target.closest('.accordion') const clickedAccordionText = clickedAccordion.querySelector('.accordion-content') lastOpenAccordion.classList.remove('minus') lastOpenAccordionText.classList.remove('open') clickedAccordion.classList.add('minus') clickedAccordionText.classList.add('open') // Handle adding and removing heights ...The closest method will look up for any first parent element that matches that selector.
When an image is an icon or non-semantic, hide it from screen readers using the aria-hidden attribute with the value true, also remember to add width and height attributes in images to prevent CLS.
<img src="assets/images/icon-star.svg" alt="star SVG" aria-hidden="true" width="40" height="40">I hope this helps! 😁
0 - @lutfiismail52Submitted about 1 month agoWhat are you most proud of, and what would you do differently next time?
I want to try using the HTML <picture> tag to display different images for different screen widths.
What specific areas of your project would you like help with?Is my approach of displaying different images for different screen sizes by using two different image elements and then setting which one to display for a given screen width correct? Or is there a better and more efficient way. Here is the code:
<div class="container"> <img class="image1" src="images/image-product-mobile.jpg" alt="a parfume" /> <img class="image2" src="images/image-product-desktop.jpg" alt="a parfume" /> <div class="wrapper"> </div> </div>@media screen and (min-width: 768px) { .image1 { display: none; } .image2 { display: block; width: 50%; border-top-left-radius: 8px; border-bottom-left-radius: 8px; } }Thank you very much for your answer and explanation. I really appreciate your time and effort in helping me! 😊
@Grego14Posted about 1 month agoHello! Congratulations on completing the challenge! 🎉
You are using the image image-product-desktop two times!
Example of use of the picture element:
<picture> <source srcset='images/image-product-desktop.jpg' media='(min-width: 768px)'/> <img src='THE-MOBILE-IMAGE-URL' width='300' height='300' alt='a Gabrielle chanel perfume'/> </picture>If the viewport width is 768px or more, it will use the image-product-desktop.
Do not jump headings! You should use the
<h1>first instead of the<h2>!When an image is decorative or non-semantic, I recommend hiding it for screen readers using the aria-hidden attribute with the value true. An example of use could be in the add to cart button icon.
I hope this helps! 😁
Marked as helpful0 - @percydocomoSubmitted about 1 month agoWhat are you most proud of, and what would you do differently next time?
Using CSS to achieve the active/focus and hover states of the project. The :has() pseudo-class is used to target elements with a more specific condition (e.g. input[type="radio"]:focus ). With the use of :has() pseudo-class, I could apply all the different style changes in this project with only CSS. If I use JavaScript to change the style, it may override the original styles initially applied so I wished not to use JavaScript to change the styles to begin with and I'm happy that it is possible.
What challenges did you encounter, and how did you overcome them?Targeting elements with very specific conditions as mentioned above. Using the :has() pseudo-class helped solved the problem. This project helps me getting familiar with this selector and the usage of it.
I had problem to have the success message and contact form page stay after submit. It seems to work after applying onsubmit="return false" in the html file, hope this is the way to solve it.
What specific areas of your project would you like help with?All feedback is welcome.
@Grego14Posted about 1 month agoHello! 😁 Congratulations on completing the challenge.🎉
To prevent the form from being submitted, you can do what you did onsubmit="return false" or you could also add an event to the form and use event.preventDefault() at the start of the handler to prevent the submission.
I would recommend not increasing the border size when doing a hover/focus state, as it results in a layout jump.
To validate if the value of an input is empty, you don't need to use input.value == "" you could simply use !input.value and if the value is an empty String this expression returns true and the code inside the if is executed, since you would be invalidating a falsy value.
I saw that you get a lot of elements to avoid having so many variables with elements, you could not call the spans and call them inside the validateInputValue function:
function validateInputValue(input) { const errorSpan = input.parentElement.querySelector('span.error') if (!input.value) { // ... } else { // ... } }
You're repeating the use of the same ID "error-text" remember that IDs must be unique!
I hope this helps!
1 - @snigdha-sukunSubmitted 4 months agoWhat are you most proud of, and what would you do differently next time?
I am proud of learning how to manipulate html content from js:
What challenges did you encounter, and how did you overcome them?selfCareCurrent.textContent = `${json_data[i].timeframes[selected].current}hrs`;I was having difficulty to make the hover function work for the ellipses, but after some google searches I was able to fix it using
:notand:hasin CSS:.content:hover:not(:has(.ellipsis:hover)) { cursor: pointer; background-color: var(--card-hover); }Responsive CSS & JS do's & don'ts
@Grego14Posted 4 months ago👋 Hello! Congratulations on completing the challenge. 🎉
After reading your code, here are some tips.
HTML
Use
<button>instead of<p>elements for the options. Since you can't focus on these elements, and you should use buttons to implement that kind of interactivity.Use default width and height in images to avoid CLS🔗.
- user_img and elipsis element.
<img src=" ./images/image-jeremy.png" alt="Jeremy Robson" class="user_img" width="60" height="60">There's no need to use an alt attribute on decorative or semantic images.
- img.icon
Use a
<h2>instead of a<p> in the cart__title element. And if you want to add the ellipse there as well, you can wrap them in a<div>.Using emmet -> div>h2+img
<div> <h2></h2> <img/> </div>Replace the
<h2>that wraps the time with a<div>or a<span>.JavaScript
You don't need to excessively repeat class deletions to elements that don't have them, you could just get the old element that contains it, remove and add it to the new one.
function updateOptionsCSS(selected) { const lastSelectedOption = document.querySelector(".option.selected_option") const selectedClass = "selected_option" lastSelectedOption.classList.remove(selectedClass) if(selected === 'daily'){ daily.classList.add(selectedClass) }else if(selected === 'weekly'){ weekly.classList.add(selectedClass) }else{ monthly.classList.add(selectedClass) } }I would rename the function getPrefix to something like updatePrefix since you're not getting it but updating it.
I recommend using data attributes in the options, Together with Event Delegation🔗 and thus avoid using an event listener for each option.
<div id="options-container"> <button type="button" data-time="daily"> <button type="button" data-time="weekly"> <button type="button" data-time="monthly"> </div>const optionsContainer = document.getElementById('options-container') optionsContainer.addEventListener("click", (e) => { if(!e.target.getAttribute("data-time")) return updateData(e.target.getAttribute("data-time")) })Event delegation is a good way to make your code take up less memory by using only one function to handle events.
Don't be afraid to use variables!
for (let i = 0; i < json_data.length; i++) { const item = json_data[i] const title = item.title const timeframes = item.timeframes[selected] if (title === "Work") { workCurrent.textContent = `${timeframes.current}`; workPrevious.textContent = `${timeframes.previous}`; }else if (title === "Play") { playCurrent.textContent = `${timeframes.current}`; playPrevious.textContent = `${timeframes.previous}`; }else if (title === "Study") { studyCurrent.textContent = `${timeframes.current}`; studyPrevious.textContent = `${timeframes.previous}`; }else if (title === "Exercise") { exerciseCurrent.textContent = `${timeframes.current}`; exercisePrevious.textContent = `${timeframes.previous}`; }else if (title === "Social") { socialCurrent.textContent = `${timeframes.current}`; socialPrevious.textContent = `${timeframes.previous}`; }else if(title === "Self Care") { selfCareCurrent.textContent = `${timeframes.current}`; selfCarePrevious.textContent = `${timeframes.previous}`; } }I hope all these tips help you, keep coding! 😁
Marked as helpful0 - @JuanMiranda1998Submitted 9 months ago@Grego14Posted 9 months ago
👋 Hello! 🎉 Congratulations on completing the challenge. 🎉
You are incorrectly using the
aria-labelattribute in the inputs. Thearia-labelattribute is used to specify a description to an element when there is nothing to describe it... as a button that only contains an icon and not text. And since you're using labels that point to the inputs you don't need to usearia-label.Since you're using
role='alert'I recommend that you read about the 👉 aria-live and 👉 aria-atomic attributes as these will help you let the user know that there's an error.I recommend using 👉 W3C Markup Validation to check for errors in your HTML code.
You're using a label tag for references to an element that isn't a form-control div#queryType.
To group elements such as radio inputs use the Fieldset tag. And since you're using radio-type inputs, you need to add the radiogroup role to the fieldset.
You're using the labels of the firstName, lastName, and email inputs to reference the same firstName element.
I hope this helps! 😁
Marked as helpful0 - @MunibAhmad-devSubmitted 9 months agoWhat specific areas of your project would you like help with?
I encountered the challenge of not correct the discount line in front of it, making it impossible to get it right for me. Additionally, I struggled to find the appropriate font family for the 'perfume' label spacing.
@Grego14Posted 9 months ago👋 Hello! 🎉 Congratulations on completing the challenge! 🎉
To achieve the line in the middle of the text you must use the property
text-decoration-linewith the value line-through, but you can also use the shorthand oftext-decoration.You're using divs elements unnecessarily to create line breaks in text:
<div class="h12">Essence Eau</div> <div class="h12">De Perfum</div>You'd better use styles to achieve that... such as paddings or margins.
The fonts you should use are in the style-guide.md file that comes when you download the challenge.zip.
Don't use multiple
h1in your code.For the text of the total price you can use a
divorspanelement instead of anh5.Use semantic elements, You are using a
divelement where you should use abuttonelement (the add to cart element).I hope this helps! 😁
Marked as helpful0 - @Jake-ElchonSubmitted 9 months ago@Grego14Posted 9 months ago
👋 Hello! To let other users see the challenge, change the URL to the .html file, as GitHub searches for a index.html file or a README.md and since yours is called frontend.html GitHub returns the README.md. You need to change the URL to
https://jake-elchon.github.io/QR-code-challenge/frontend.html.And for the image to load you must use a relative URL like this
src="./image-qr-code.png"because if you usesrc="/image-qr-code.png"(without the dot) you would be looking for the image inhttps://jake-elchon.github.io/image-qr-code.pngand it doesn't exist.Congratulations and hope this helps! 😁
0 - @shivPostedSubmitted 9 months agoWhat are you most proud of, and what would you do differently next time?
I am proud to have achieved what I hoped in the project The changes I would make next time are to improve or optimize the code 😄😄
What challenges did you encounter, and how did you overcome them?The most challenging part was to display user's and computer's choices on the viewport I achieved it by using insertAdjacentHTML method
What specific areas of your project would you like help with?I would like to get help with how I can optimize my code
@Grego14Posted 9 months ago👋 Hello! 🎉 Congratulations on completing the challenge! 🎉
Use ids to get the elements in the JavaScript code.
To avoid using a lot of eventListeners you can use 👉 Event Delegation and this would optimize the code and make it more readable.
Use semantic html tags, you're using a
divelement for the rulers button when you could use abuttonelement and that causes that users are not be able to focus on the element using the keyboard.add the
tabindex='0'attribute to all the choice--icon so that you can focus an option using the keyboard and add a keydown event for select an option using keys likeEnterorSpace.To avoid repeating the code in the eventListener of the closeRuleBtn and the rulesBtn you can do something like this:
function handleButtons(){ rulesOverlay.classList.toggle('hidden') bodyOverlay.classList.toggle('hidden') }The classList interface toggle method removes or adds the class depending on whether it contains it or not.
I hope this helps! 😁
Marked as helpful0 - @VidottizzzSubmitted 9 months agoWhat are you most proud of, and what would you do differently next time?
i completed the validation of the form, and then when every input are filled, the alert show a message saying that it is completed, but i used the preventDefault to prevent the form being submited, and i dont know how can i stop the preventDefault from working after the inputs are completed, so the form isnt sending nothing, can someone help me ?
@Grego14Posted 9 months ago👋 Hello! 🎉 Congratulations on completing the challenge! 🎉
The preventDefault() method is used in a form when you don't want it to be sent, in this case you want it to be sent when everything is correct.
To prevent the form from being submitted when there is an error, you can use the preventDefault() along with validations, something like this:
nameValidate(); lastNameValidate(); emailValidate(); radioValidate(); checkBoxValidate(); if (!(checkBox.checked === true) && !(fields.value.length > 0) && !(fields.value.length > 0) && !(emailRegex.test(fields.value)) && !(htmlRadio1.checked || cssRadio2.checked)) { return e.preventDefault() } alert("Message Sent. Thanks for completing the form. We'll be in touch soon!");Here we would be using all the validation functions before verifying that the form is correct, since those functions remove and add the errors.
And we use
return e.preventDefault()in validating whether the inputs are invalid to prevent the alert from being sent.Using the tag
bris considered a bad practice, as it should be used only when breaking the line is necessary, such as a poem of a few lines. It's best to use styles to achieve what you want.When using multiple inputs in a container, it's best to use the 👉 Fieldset tag instead of a div.
You have duplicated ids in two input elements lname.
No need to use the type attribute in the
scripttagI hope this helps! 😁
0 - @ShoaibShujaSubmitted 10 months agoWhat are you most proud of, and what would you do differently next time?
Once again, I designed the layout easily by the CSS's amazing flexbox feature which did like 60% of the page done. I also figured out how to make a page's background half image and half color which I am sure would be useful in many cases. And finally, I created a JavaScript function which would toggle the display of the answer text on clicking the plus or minus icon and would change the plus or minus icon to opposite according to the display.
What challenges did you encounter, and how did you overcome them?The half image and half color background was a rare challenge but I figured it out by learning a bit more on image backgrounds which background position and size came in handy. The function which would toggle the display of the answer text was also a bit of a challenge, but I figured it out easily by using if and else.
What specific areas of your project would you like help with?Some more info about the layout's certain designs would be useful especially in the style guide's file.
@Grego14Posted 10 months agoHello! 🎉 Congratulations on completing the challenge! 🎉
Something I recommend is to apply the line-height property to the
:rootwith a value of 1.2-1.5 as it will help to read the text better. And maybe a little bit ofletter-spacinglike 1px or half a pixel.If the images are icons or are for decoration, add the aria-hidden attribute to remove them from the accessibility tree. 👉 aria-hidden
Since the accordion is opening using the icon you should add a more specific alt attribute, something like "Click to open the accordion" as people with screen readers will read "plus icon".
That's all! I hope this helps you. 😁
Marked as helpful0 - @gabrielcarreniomaker33Submitted 10 months agoWhat are you most proud of, and what would you do differently next time?
I am proud to have worked with CSS media queries.
What challenges did you encounter, and how did you overcome them?The challenge was to work with CSS media queries so that the component does not break its fluidity when it grows.
What specific areas of your project would you like help with?I would like to receive help to know if I am practicing good CSS practices.
@Grego14Posted 10 months ago👋 Hello! 🎉 Congratulations on completing the challenge! 🎉
Remember to specify a more descriptive alt attribute, as it is commonly used by screen readers, no one wants to listen to 'photo profile'. Something like this 'Jessica Randall photo profile' would look better.
You're using two media queries unnecessarily. You have two for 600px and two for 750px.
Move the media querys to the end of the file and add the styles from the a element to the media query where you add the styles to the .main-container.
@media screen and (min-width: 750px) { .main-container { padding: 2rem; height: 610px; width: 415px; } a { font-size: 15px; padding: 16px 0; } }This would save code and simplify the use of media queries.
hope this helps! 😁
Marked as helpful0 - @AngelaPetreskaSubmitted 10 months agoWhat specific areas of your project would you like help with?
What would you do differently in this code, especially JavaScript? Thank you in advance!
@Grego14Posted 10 months ago👋 Hello! 🎉 Congratulations on completing the challenge! 🎉
HTML:
I recommend using the
timeelement to specify the time in the challenge, it would look something like this:<time datetime="2020-06-28">28 Jun 2020</time>Read more about the time element here -> time
CSS:
Remove the
max-height: 28remfrom the main element as it causes the footer to overflow.JS:
I recommend creating a handler outside of the for loop because otherwise you would be creating an eventListener for each shareicon and on large websites this can cause performance problems.
You should take a look at -> Event Delegation.
the
togglemethod of the classList interface is used so that if the element contains that class, it removes it, if not, it adds it.If you want to know more, check out the documentation -> toggle
const theListener = (e) => { share.classList.toggle('slide-out-bottom') share.classList.toggle('slide-top') } for (let i = 0; i < shareicon.length; i++){ shareicon[i].addEventListener("click", theListener); }I hope this helps! 😁
0 - @RahulDev118Submitted 10 months ago@Grego14Posted 10 months ago
Hello! 🎉 Congratulations on completing the challenge! 🎉
Remember not to skip the headings, you should use a h1 instead of a h4.
Replace that h6 with a p to make the text more semantic.
Since you're not adding an alt attribute with description, you can use the aria-hidden attribute to hide the image from the accessibility tree.
Hope this helps! 😁
Marked as helpful1 - @JohnPugh688Submitted 10 months agoWhat are you most proud of, and what would you do differently next time?
Finding the tag which allowed me to add the dropdown for tag without the need for any javascript.
What challenges did you encounter, and how did you overcome them?i couldn't figure out ow to transition the drop down accordion to make it look a little bit smoother using tailwind. I'm sure there is a way but i just couldn't seem to find a solution so if anyone knows how to get that to work please drop me a message please
What specific areas of your project would you like help with?If someone knows how to add a transition to the drop down accordion using tailwind css please drop me a message. thanks in advance
@Grego14Posted 10 months ago👋 Hello! 🎉 Congratulations on completing the challenge! 🎉
There is no way to transition from closed to open state without using javascript, as the documentation says "there's no built-in way to animate the transition between open and closed."
Leave the alt attribute empty if you are not going to specify a descriptive text or if the image is just for decoration, when you use it make sure to specify a descriptive text since it is used by screen readers, it should be something like "close the accordion" and not "plus icon". Remember that you can also use the aria-hidden attribute to hide icons and remove them from the accessibility tree.
I hope this helps! 😁
Marked as helpful0 - @metinahmeterkelesSubmitted 11 months agoWhat are you most proud of, and what would you do differently next time?
I'm happy to code another design. I learned many different new things on this journey. I am happy that I can code the design for mobile and desktop screens.
What challenges did you encounter, and how did you overcome them?I thought a lot about whether I should start coding from mobile design or desktop design. I think starting from mobile design is a much better solution.
What specific areas of your project would you like help with?Responsive layout
@Grego14Posted 11 months ago👋 Hello! 🎉 Congratulations on completing the challenge! 🎉
I recommend you remove those heights that you placed on the .container element and also that
overflow: hiddensince on smaller screens or horizontally placed devices it can cause overflows and non-visible content.I think you forgot to remove the text from the alt attribute in the image, also remember to add it if the image has a semantic meaning or is not for decoration. And if you add it, remember to use descriptive text.
To prevent text wrap in the span element inside the button you can use something like this:
max-width: max-contentinstead ofmax-width: 85pxor just remove that max-width!I hope this helps! 😁
Marked as helpful0
