@5hraddha
@5hraddha
Submitted
Hi Everyone,
I have used ReactJS and Styled Components to code for the landing page. Could anyone please have a review of my code and the live webpage and let me know if the Styled Components have been used efficiently and point out the areas where I could improve. Thank you!
Regards, Shraddha
@5hraddha
Posted
Hey @skyv26,
I got the problem resolved. Thanks for pointing out that you were seeing at different code while inspecting my webpage and the issue was that although I fixed the code but didn't deploy the code again on gh-pages. So, nothing was getting updated. My bad!!
Thanks again for your time! I completely forgot and was scratching my head.
@MarkWasfy00
Submitted
@5hraddha
Posted
It is my pleasure @Markizzo. Take care..
Marked as helpful
@MarkWasfy00
Submitted
@5hraddha
Posted
Hey @Markizzo, Good job on implementing the design. π There are a few suggestions to improve upon:
I appreciate that you have used <header>, <main> and <footer> semantic elements. But, you could improve further by using <section>, <article> and <ul> tags. It gives better meaning to the markup and improves accessibility.
Freecodecamp Semantic Elements.
For more clarity on nesting of HTML elements semantically, you could refer this article - CSS Tricks - How to section your HTML Elements. I usually refer this when I am confused which elements should be nested in which ones.
Also, it is good to give a meaningful value to alt attribute of <img> tag.
It is good to have an understanding that <div> is a container tag. And it should not be used for writing headings and paragraphs. You have <h1>, <h2>, ..., <h6> tags for headings and <p> tag for paragraph. You could correct the usage of tags in cards.
In footer, you have a list of links (both social icons and texts), you have to give them in <a> tags with proper value of href attribute pointing to a meaningful URL.
For every even no. card, the position of image is reversed in the design. You could achieve this using flex box's property flex-direction as row-reverse.
Thank you!!
Marked as helpful
@doomware
Submitted
@5hraddha
Posted
Hey @doomware, Great job at implementing the design π There are a few suggestions for enhancements:
<>.. . It is the shorthand for using React Fragment.I appreciate how clean and crisp your solution is for the design. Keep it up β
Marked as helpful
@leodsc
Submitted
@5hraddha
Posted
Hey @leodsc, Awesome job at implementing the design. I really like how you have used different React features together.
There a few suggestions for improvement that means nothing is wrong but in case, you wish to level up and polish your solution.
Ensure headings are in a logical order. For example, check that all headings are marked with h1 through h6 elements and that these are ordered hierarchically. For example, the heading level following an h1 element should be an h2 element, not an h3 element.
You could use the JS object destructuring syntax to get values from props in a component. Nothing big, but it makes code more clean and you get to use new features of the language. Here, is a link of an awesome article on the same - Object Destructuring in JS
I too learnt few things while going over your code. Thank you and have a great day!!
@leodsc
Submitted
@5hraddha
Posted
Hi @leodsc, First of all, congratulations for completing the challenge. It is a great job indeed.
There are a few observations that I would want to share and suggest some improvements therefore.
The webpage should have a main heading - <h1> tag. You have kept the page header in <header> tag which is a section tag. That means it just defi es and landmarks a particular section of your webpage as a header. It is more similar to a container. Now inside that container, you have to also wrap the heading in a <h1> tag. That would remove one of the error in the report.
Ensure headings are in a logical order. For example, check that all headings are marked with h1 through h6 elements and that these are ordered hierarchically. For example, the heading level following an h1 element should be an h2 element, not an h3 element..
You have <label> for the <input> in your code but you have not associated them. You can use id and htmlFor attributes to do the same.
Thank you!! βοΈ
Marked as helpful
My first project: Since I'm new, i would like to have some general feedback on my project.
My specific questions:
@5hraddha
Posted
Hi @arnameti, Congrats on completing your first challenge. You have done a great job implementing the design.
As far as BEM methodology is concerned, there are certain things to improve upon. You have to be careful in understanding when to use element and when and how to use modifiers.
So, in your case the naming - card__days--left is okay as it is changing some property of the original element but card__days--img is not a correct name for a BEM modifier. Here you are not changing any characteristics of the element - card__days.
Also, keep in mind that a BEM modifier cannot be used alone. It is like an adjective used to enhance the character of a noun. So, it will always be used with a BEM Element (noun) first followed by its BEM modifier (adjective). For example: This is the correct usage -
<div class="block block--mod">...</div>
<div class="block block--size-big
block--shadow-yes">...</div>
While, this is wrong -
<div class="block--mod">...</div>
Also, ideally a BEM modifiers should not be used for positioning elements. If you have to position an element, you should use BEM mixing.
You could read more about it - here
Thank you!!
Marked as helpful
@rosberg03
Submitted
@5hraddha
Posted
Hey @rosberg03, I want to appreciate you for completing the challenge and the beautifully written code that is so easy to go over and understand. I loved the structure of the files and even though you have written the code. Great job!!
There are just two things I would like to suggest:
You have done a nice trick in splitting the page heading into two lines. But, I was wondering that separating the word 'SPLITTER' by a space would force the screen reader to read the word as two separate words like - SPLI TTER. That is not a good idea. I believe that if we can keep the word as one, and use <span> tag to break it into two lines, it would be great. As screen readers, just don't take any meaning out of <span>. So, it is better to make use of it for styling purposes.
Also, in most of the part you have implemented BEM methodology in a crisp manner but at two places, two elements of different blocks are nested within each other. If somehow we could avoid that, it would be even more clear and good to read and understand the BEM blocks and elements.
Great job!! β Thank you!!
Marked as helpful
@Abdul400
Submitted
Hello Guys, any feedback on my work would be appreciated. Thanks a lot :)
@5hraddha
Posted
Hi @Abdul400, First of great job in implementing the design. πThere are a few suggestions that would like to give for improvements, if you would like:
Instead of using <div> tags as a container every time, you could look into using different HTML semantic tags. For example - you could put logo in <header> tag and rest of the hero section of the webpage in <main> tag. It gives better meaning to the markup and improves accessibility.
Freecodecamp Semantic Elements.
For more clarity on nesting of HTML elements semantically, you could refer this article - CSS Tricks - How to section your HTML Elements. I usually refer this when I am confused which elements should be nested in which ones.
The social links are not positioned properly. You could try to put them in the bottom-right corner as given in the design. But, what I understand is that the social links are supposed to be hyperlinks that when clicked should take you to the respective pages. So, it would be good to wrap icons in <a> tags and style them such that when hovered over, the cursor should change to pointer.
Marked as helpful
@iddimsangi
Submitted
Hello Team! I've just completed this challenge, i will appreciate your comments or any suggestions.
@5hraddha
Posted
Hey @iddimsangi, First of all, great job on implementing the design!! πThere are a few suggestions that would like to give for improvements, if you would like:
If you write a lot of JavaScript, then writing variables in camel case is a common practice. So, for naming CSS classes, it is good to use Hyphen Delimited strings by convention (all in small case).
I could see that you have used a <div> tag as footer inside a <header> tag. Syntactically, there is no problem but semantically, this is not correct. In a webpage, we should not nest a footer inside a header. For more clarity on nesting of HTML elements semantically, you could refer this article - CSS Tricks - How to section your HTML Elements
Also, instead of using <div> tags as a container everytime, you could look into using different HTML semantic tags. It gives better meaning to the markup and improves accessibility.
Freecodecamp Semantic Elements
Great job. Enjoy the learning.
Marked as helpful
@MathisHumbert
Submitted
I coded this project using React, Redux and Styled Components.
This is my first project using Styled Components, I really enjoyed it and it's so easy and convenient to use.
Feel free to comment my code!
@5hraddha
Posted
Great job @MathisHumbert. π
@sisodiaa
Submitted
@5hraddha
Posted
Hi @sisodiaa, Congrats on completing the challenge and doing a great job at implementing the design. I appreciate your clean indented code writing style. π
There are a few things that I want to suggest in case you want to look for further improvements (either in this challenge or the next):
Instead of using <div> tags as a container everytime, you could look into using different HTML semantic tags. It gives better meaning to the markup and improves accessibility.
Freecodecamp Semantic Elements
Since, you have used BEM naming convention for CSS class names, you could also divide and modularize your css file into several smaller files and arrange them according to BEM File Structure Organization Methodologies. This would be beneficial for large projects especially. You could look into this link - BEM File Structure
Marked as helpful
