@TdB27Submitted over 3 years ago
My JS was very repetitive, can you abbreviate it?
Sebastian Porini
@zebokAll comments
- @zebokPosted over 3 years ago
Hey Thiago! First of all: GREAT CHALLENGE SOLUTION!!!. Well done mate. Really well done. Congratulations.
About your request regarding your js file, i have an idea.
On first place, to avoid using the for loop, i would target the HTML elements by class or by id. Instead of using querySelector or querySelectorAll.
So, instead of:
const activeButtonProduct = document.querySelectorAll("#link-product"); const activeButtonCompany = document.querySelectorAll("#link-company"); const activeButtonConnect = document.querySelectorAll("#link-connect");
Better do:
const activeButtonProduct = document.getElementById("link-product"); const activeButtonCompany = document.getElementById("link-company"); const activeButtonConnect = document.getElementById("link-connect");
As i can see, you use anonymous functions right after the "click" event listener. And all the three anonymous functions do basically the same. Toggle 2 classes.
So, on second place, i would give that funcion a name, and calling it by its name right after the click event listener I have called the funcion "active". But you can choose the name you want.
Instead of:
element.addEventListener('click', function () { menuActivedProduct.classList.toggle('show-active') activeOpacityProduct.classList.toggle('opacity-actived') })
Better do:
element.addEventListener("click", active)
And, finally, the 3rd step. Depending on wich element is firing the functions (this), the function will work differently. I have used a switch statement. Here is the function.
function active() { switch (this) { case activeButtonProduct: menuActivedProduct.classList.toggle("show-active"); activeOpacityProduct.classList.toggle("opacity-actived"); break; case activeButtonCompany: menuActivedCompany.classList.toggle("show-active"); activeOpacityCompany.classList.toggle("opacity-actived"); break; case activeButtonConnect: menuActivedConnect.classList.toggle("show-active"); activeOpacityConnect.classList.toggle("opacity-actived"); break; } return; }
Cheers! Keep going my friend! Some day we will be experts doing this! If you have more questions, just ask and i I'll be happy to help you.
Un abrazo enorme desde Argentina a Brazil! Amo Brasil y su gente. El mejor pais del mundo. <3.
PS: Try using the same logic, on the hamburger/cross button. Thats your homework for tomorrow. Hahah.
0 - @tony1610Submitted over 3 years ago
All comments are welcome! Grid and Grids / No JS / have not learn JS yet. More focus in HTML and CSS.
@zebokPosted over 3 years agoHey Tony! Well done mate! The layout looks good. Congrats!
Try using the correct HTML elements.
There are some semantic HTML elements (specifics) (<article>, <main>, <footer>, <section>, etc) And! Non semantic HTMl elements (non specifics) ( <div> <span>)Try using them corectly. This is very useful. Semantic element describes its meaning to both developer and browser.
I can see you have used the <article> HTML element for just a paragraph. In that case, you should use the <p> HTML element.
You have also used <section> HTML element inside a <div>. This is weird. Usually we use the <section> HTML element precisely for that, a section. Inside this <section> you can have articles, divs, h1/h2..., paragraphs, forms, etc.
Michelle Appleton 28 jun 2020
This is not an article, this should be a div. With 2 <p> tags, or maybe one heading and one <p> tag.
Try using the correct HTML elements. Think about it. Ask yourself. What is this? It is.. an article? O maybe its a list? Help the browser understand.
If you cant define what it is with a semantic HTML element, then use a div or a span.
Hope you understand me! Keep practising mate! Keep going! Some day, we will we experts doing this! Cheers!
Marked as helpful1 - @srikartopellaSubmitted over 3 years ago@zebokPosted over 3 years ago
Hey Srikar! Well done mate! The layout looks good and the calculator works perfect. I like all the hover/focus effects and the bug report button.
I just have 2 little comments.
1: You should not use the same ID twice. IDs are meant to me unique. You are using the id "btn" for all the buttons. This is not the best approach.
2: When i click the anchor tag "Srikar Topella" rigth at the bottom of the page, it returns me to the same page. It would we better if you could redirect that link right to your github profile or your personal website.
Thats all mate! Well done! Keep practising. Some day we will be experts doing this :)
Marked as helpful1 - @Nam-HaiSubmitted over 3 years ago
Although I'm very proud of this, there is something that bothers me.
When the site is first loaded, the button doesn't react imediatly. Which is wierd. If someone knows why, it would be awsome.
@zebokPosted over 3 years agoHey Nam! Well done!!
I like that smooth effect! Great idea!
Just a little tip: Do not place a "main" element inside a "div" element. This is not the best approach talking about accesibility. The best way is setting a "main" element and inside a "section" element.
0 - @ajdonatoSubmitted over 3 years ago
This is my first challenge done in HTML and CSS. All feedback is welcome.
@zebokPosted over 3 years agoHey Donato!
Desktop version looks great, well done. Otherwise, mobile and tablet version are not the best. Specially tablet version. Check about the overflow in the page. It looks like everything is going out of the window/browser. Try adding some max-width property.
I have 2 tips for you: First one: Never use margins to position an element. Just use margin to give a space between elements.
Second one: Check out about the "picture" tag and how you can set different images with the srcset attribute. The srcset attribute is used to offer list of possible images based on size.
Here you have a little example. The img element will take "small.jpg" as the scr attribute.
UNLESS! the screen width is 800px or more. In that case, the image will take the "big.jpg" as his src attribute.
<picture> <source srcset="big.jpg" media="(min-width: 800px)"> <img src="small.jpg" alt="" /> </picture>So you will have differents images depending on the screen width. And, with grid property: "order" you can set the child to go first or after. And place it correctly in the body. The idea of setting display="none" to the card-img classes is not the best approach. You can read more about picture and srcset here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/picture
Thats all mate! Keep practicing! Hope my tips help you. I am learning as well. Some day, we will become experts ;)
Marked as helpful1 - @jeffkobbySubmitted over 3 years ago
Any feedback will be appreciated. Kindly give tips on how to optimize the responsiveness of the page. Thank you
@zebokPosted over 3 years agoHey Jeffrey!
Check out the font size in desktop version. Also, you can change the font-family in the placeholder with ::placeholder pseudoelement. In your case would be:
#inputEmail::placeholder { } Check out more here: https://developer.mozilla.org/en-US/docs/Web/CSS/::placeholder
Another issue i have found in your version, is about the click. When i click the button, the error icon and the error message shows and dissapear. Check that.
And... (the last one).
Usually, the content never reaches the total width in desktop version. So, add some padding, margin, whatever you want. The idea is to leave a little space between the content and the screen border.
Thats all! Keep doing it! Keep practising! Some day, we will become experts :)
Marked as helpful1 - @costelloewardSubmitted over 3 years ago
Hi everyone, any feedback much appreciated. This was my first challenge using grid. The design asked for a hover state to apply to the button, but I used focus as this is better for keyboard users.
Is there a way to simplify the border-radius code I'm using to get the rounded corners?
@zebokPosted over 3 years agoHey Louise!
Try adding a max-width property. So the divs wont grow that much.
Also, try something else with the media-queries. Right not is not working that nice around 1000px screen width.
Marked as helpful0
