@termjsSubmitted over 2 years ago
Is there anything I could improve in the code?
Arthur Roberts
@arfarobsAll comments
- @arfarobsPosted over 2 years ago
Hey. Great work so far Casparas.
I suggest wrapping your <div class="qr-code"> with a <main> element. This will improve accessibility.
When using headings, you should always start with a <h1> and work your way down from there. If your document doesn't have a <h1> heading, then it shouldn't have a <h2> heading. I suggest replacing the <h2> with an <h1> element.
Hope it helps.
1 - @arfarobsSubmitted over 2 years ago
I found the design of this fairly simple. I used this website to learn about web development tools that were new to me. I learned so much by doing this project.
It's the first time that I have ever tested my own react app. I find it really difficult to know what I should test. Any feedback on whether I have missed something that should be tested or testing in general would be great.
It's the first time that I have used the following tools on my own project. Any feedback with these would be great.
- framer-motion
- react-testing-library
- Jest
- Storybook
- husky
I'm looking forward to my next project. I'm hoping to implement an external database using something like Firebase.
Issues
- When the nav links are spammed (navigating through different pages really fast), the content and enter animations fail. The user has to refresh the browser to get the content to display properly. I have no clue whatsoever as to why this is happening.
Any questions or feedback would be great.
Also, let me know what I can do to help you. Leave a message in your feedback with a project that you would like my input on.
@arfarobsPosted over 2 years agoP.S I've just noticed the planet image size, HTML, and accessibility issues. I will fix them over the next few days.
0 - @JorahhhSubmitted over 2 years ago
Hey there people!
Here is probably my best project here on Frontend Mentor. I've been on this for awhile and the result is fine.
I've tried with all my knowledge to do as clean as possible the code.
Super proud of it, especially for the grid layout where I studied and improved on how to set it, the columns, the rows and the gap. Used the flex layout too, only for the mobile version.
I just changed few things from the original design (h1,h2, font-weight)
I would appreciate some feedback on it :)
EDIT: Just updated my code. I replace the old map (the image) with the "interactive" one from leaflet with js (using a tutorial, I'm still not good with js) and changed the <button> (now the Report shouldn't report any issue with the code) and its hover effect. In addition I changed the grids margins. Now it looks better, I guess :)
@arfarobsPosted over 2 years agoHey Angelo. I'm currently working on this project myself. I like the hover effects you put on the images.
I've got a suggestion to neaten up your code with the grid images. It's not good to have three <img> tags for the same picture. This has something to do with accessibility and SEO. I spent hours looking for a solution to this. I will share it with you.
Instead of this:
<div class="mobile-grid1"> <img src="assets/mobile/[email protected]" alt="grid1"/> </div> <div class="tablet-grid1"> <img src="assets/tablet/[email protected]" alt="tablet-grid1"/> </div> <div class="desktop-grid1"> <img src="assets/desktop/[email protected]" alt="desktop-grid1"/> </div>I'd suggest using this:
<picture class="img-grid-1"> <source media="(min-width: 1440px)" srcset="./resources/assets/desktop/image-grid-1.jpg"> <source media="(min-width: 768px)" srcset="./resources/assets/tablet/image-grid-1.jpg"> <source media="(max-width: 375px)" srcset="./resources/assets/mobile/image-grid-1.jpg"> <img src="./resources/assets/mobile/image-grid-1.jpg" alt="The galleries main room."> </picture>This will avoid confusing search engines. Also, the pictures will automatically change to the correct file without adding them to your media queries in CSS. If I'm correct, I think this will also help loading speeds because the browser will only load the image that it needs, but i could be wrong about that.
Hope it helps.
Marked as helpful1 - @vedatsozenSubmitted over 2 years ago
I had difficulties when i am trying to put contents to divs. Aligning was a little bit difficult. I have to learn to use flex more.
@arfarobsPosted over 2 years agoHey Vedat. It's looking good. I have a few suggestions for you.
Accessibility:
-
All of you images should have an alt attribute with a description of what is in the image as a value. For decorative images like the icons, Id suggest giving them an empty alt attribute. https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement/alt
-
As Shashree mentioned above, your content should be wrapped in a <main> element. I also noticed that you wrapped the heading in a <p> element. I suggest that you change that to an <h1> element. All webpages should have an <h1> element. The link below explains the best practices when using headings. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements
Flexbox:
- You said you had issues with flexbox. I will leave two links below. These links are games in which you use flexbox as the controls. They are great for practicing flexbox. They're also quite fun and explain a lot to you. https://flexboxfroggy.com/ http://www.flexboxdefense.com/
I hope this helps you. I have also completed this challenge. I used flexbox in my solution. Feel free to refer to it.
Marked as helpful0 -
- @GreenDiggySubmitted over 2 years ago
The biggest struggle I had was the sizing of elements and having proper ratios to match with the design pictures. What are some more effective ways of approaching the scales and sizing when basing the design off of a static image?
@arfarobsPosted over 2 years agoHey. If i was designing this just from the picture. I would use the text as a method to scale a close size. In the downloaded files for the project there should be one that tells you what styles to use. This will tell you the font sizes, colors, etc.
In the design picture you can see that the heading is two lines. After the word "front-end" you will see that the line breaks. By looking at it this way you can make a better estimation for the components width.
In a project like this i think its best to try and style things in relation to the information that you are given in the style guide.
Marked as helpful1 - @JorahhhSubmitted over 2 years ago
Used Grid for this challenge for the first time.
Really though for me, and a I don't even know if it's a good code or if I could do It better. Any advice on how do it better would be appreciated.
@arfarobsPosted over 2 years agoHey, Angelo. Great job on learning the basics of CSS Grid. I have a few suggestions for you, mate.
-
If I was to do this project, I would put the line "join our community" as an <h1> heading. This will avoid the accessibility error in the report.
-
To fix the issue with the media query, you can change line 126 from this: @media (max-width: 1600px) and (min-width: 800px){ to this: @media (min-width: 800px){ By doing this you are saying that you want your media query to work on all screen sizes with a width of 800px+.
Marked as helpful0 -
- @JorahhhSubmitted over 2 years ago
Okay, guys! I did this challenge using Flexbox....but I would like do it again with the Grid, but I'm still not into it.
I'd love someone to give me some advice on how to set it up, using always the same HTML lines code.
@arfarobsPosted over 2 years agoThis might be useful for you. It's a game that will go through the basics of CSS grid.
https://cssgridgarden.com/
1 - @HeronChengSubmitted over 2 years ago@arfarobsPosted over 2 years ago
下午好呀！看起来很棒。I have a few suggestions for you.
-
I suggest adding some attributes to the <img> icons. <img src="./images/icon-sedans.svg" alt="" aria-hidden="true"> If an image is decorative, you should add an empty alt attribute. Then, add an aria-hidden="true". This will hide the element from accessibility tools.
-
I suggest wrapping your content in a <main> element. Landmark elements are important for accessibility.
-
I noticed that nearly everything is wrapped withing <div>s. It would be better to use some other elements.
- For headings use: <h1> <h2> <h3> <h4> <h5> <h6> in that order.
- For paragraphs use: <p>
- For buttons use: <button>
I hope this helps. 因为我注意到你来自台湾，所以我觉得你会说中文。下面有两个网站， 第一个是真的很有用，有中文和英文。
- https://developer.mozilla.org/zh-CN/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-hidden_attribute
- https://www.w3.org/TR/wai-aria-practices/examples/landmarks/HTML5.html
Marked as helpful0 -
- @Harelk1015Submitted over 2 years ago@arfarobsPosted over 2 years ago
Hey Harel. I have a few suggestions to help you improve your solutions.
-
The image doesn't display properly. I noticed that there is an error in the <img> src attribute. Your code: <img src="/images/image-qr-code.png" class="card__img" alt=""> Corrected version: <img src="./images/image-qr-code.png" class="card__img" alt=""> The first / needs a full stop in-front of it.
-
To avoid the errors in the report, there are two things that you can do.
- Wrap your HTML in a <main> element.
- Instead of using an <h3> element use an <h1> element. Headings are supposed to follow a kind of hierarchy system. If you only have one heading element, it should be an <h1>. Every page should only have one <h1> element. Then after that use <h2> then <h3> <h4> etc. depending on the importance of the heading.
Marked as helpful1 -
- @JorahhhSubmitted over 2 years ago
Okay, guys! I did this challenge using Flexbox....but I would like do it again with the Grid, but I'm still not into it.
I'd love someone to give me some advice on how to set it up, using always the same HTML lines code.
@arfarobsPosted over 2 years agoHey Angelo. I'm not the best with grid, so i can't answer that question. However, I have a suggestion when it comes to accessibility.
I notice that you have wrapped your HTML in a <div class="main">. To avoid getting the errors in your report, it would be better to wrap it in a <main> element.
Marked as helpful0 - @arfarobsSubmitted over 2 years ago
This is the first time that I have done a Frontend Mentor project with JavaScript. Any feedback would be great, especially on the JS or accessibility side of things.
@arfarobsPosted over 2 years agoAlso, I always seem to have problems with my typography. Any feedback on how to improve that would be great.
0 - @ackuserSubmitted almost 3 years ago
Hello FrontEnd Mentors and *,
I would like to know if everyone has an exact correspondence between this (see image below) in Sketch and CSS.
I mean, how do you translate exactly into css Character, Line and Paragraph?
https://freeimage.host/i/lmin0x
Any trick for hits different devices with media queries? I am really struggling when I have to check MDPI, HiDPI or my Macbook Pro 15' and everything with high resolution
https://freeimage.host/i/lmioUQ
Any help for future developments would be really appreciated it, happy coding!!!
@arfarobsPosted almost 3 years agoHey Karim. I just completed this project. I used Figma as apposed to sketch.
To try and answer your question. I would assume that character stands for letter-spacing in this case it doesn't have a value. Line would stand for line-height and in your pictures case it would have a value of 28px. Paragraph stands for paragraph-spacing in your pictures case has a value of 1.
I hope I've read and understood your question properly.
Marked as helpful1
