@JuanMiranda1998Submitted 5 months ago
Grego
@Grego14All comments
- @Grego14Posted 5 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 5 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 5 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 5 months ago@Grego14Posted 5 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 5 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 5 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 5 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 5 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 5 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 5 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 6 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 6 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 6 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 6 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 6 months ago@Grego14Posted 6 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 6 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 6 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 6 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 6 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 - @Ghm84Submitted 6 months agoWhat are you most proud of, and what would you do differently next time?
I'm happy with the result since it's the first repository I've made.
What challenges did you encounter, and how did you overcome them?After I've finished the image didnยดt show up, so I had to make some changes.
What specific areas of your project would you like help with?The responsive design.
@Grego14Posted 6 months ago๐ Hello! ๐ Congratulations on completing the challenge! ๐
Remember not to use the alt attribute when the image has no semantic meaning or is only for decoration. And when you use it, you should write a more descriptive text.
And if you decide not to use an alt attribute, remember to remove the image from the accessibility tree, the
aria-hiddenattribute will help you with that.Do not use the br tag to break texts, as it is considered a bad practice and can lead to accessibility problems, read more here ๐ br.
I don't recommend using a fixed width in elements like the body, unless you want to make some kind of website where all the content has a few spaces on the sides, but for something like that it's better to use padding.
I recommend that you use min-height instead of height as that will allow your content to grow if necessary.
I hope this helps! ๐
Marked as helpful0
