@scarlosteixeiraSubmitted over 1 year ago
I'd like to heard from you any comments about best pratices and improvements that I should apply on my code.
P
Darkstar
@DarkstarXDDAll comments
- @DarkstarXDDPosted 17 days ago
As part of the learning paths, i was asked to give some feedback on your solution. Couple of suggestions i have.
- You can switch to the desktop layout bit more earlier, so mid size screens won't see lot of empty space.
- Currently the error message is displayed as soon as i type the first letter in the input. I think it's a bit annoying. You should allow the user to try and submit at least one invalid entry first, before showing any errors. The ideal option would be to validate
onblur.
0 - @AutumnsCodeSubmitted 5 months agoWhat are you most proud of, and what would you do differently next time?
I am really proud how smoothy the JS came along. I had been worried that it might take me a little bit longer. I would like to structure my css more. Sure, I could use SCSS or Co. but I didn't believe it was neccessary for this challenge.
What challenges did you encounter, and how did you overcome them?Well, the challange came with Netlify. When I run
What specific areas of your project would you like help with?npm run buildthe JS file wasn't exporting into the dist file and so the function didn't work either. I did some testing outside then I realised I needed to addtype="module"into the script file.Any feedback is welcome!
@DarkstarXDDPosted 21 days agoHey. Looks like this is a 4 month old submission but somehow in the learning paths i was asked to give feedback on this. Overall it looks good, but here are some suggestions i have.
- There is no need for a
<header>and a<footer>in this challenge. And even if there was, those two should not go inside the<main>. They should be outside of the<main>. This is just a component, so<main>should be the only landmark element needed here. <header>and<footer>both are usually used to wrap content that repeats across multiple pages in a website.<header>you usually see at the very top of a site containing the site logo and the navbar (site navigation).<footer>at the very bottom usually contains any attribution of the site, some contact info, a sitemap (secondary site navigation) etc.- When a button or a link only has an image/icon inside it and no text, give it a
aria-label. That way screen readers can announce that name when a user navigates into that link. Otherwise the user won't know what that link/button will do since there is no text on it to be announced. You have already given anaria-labelto the share button, you can do the same for the social media links as well. - I think there is no need for the alt text on that user avatar. Just reading out the name of the person doesn't make that much sense because you already have the name of that person right next to it. So the screen reader will be just reading the name of the person twice. Also at the same time it's very small that there is no point trying to describe that image either. So i think you can keep the alt text of that avatar empty.
alt="". - Currently when the site loads, the share dialog pops up and then disappears. When i initially click on the share button it doesn't open the dialog. Only when i click the second time the dialog opens. This only happens when the page first loads.
0 - There is no need for a
- @TheWraithDevSubmitted 28 days agoWhat specific areas of your project would you like help with?
Responsiveness
@DarkstarXDDPosted 28 days agoLooks nice. Some suggestions.
- Always use
remforfont-sizes. That way if the user changes their default browser font-size, the text in your website will scale accordingly. This won't happen if you usedpxfor font-sizes. - Try to avoid having text inside
divorspanalone. They are meaningless elements used for grouping or styling purposes. Change that quote text to apelement or better, for ablockquoteelement. You can usespanif you want to select some text that is already inside ap. - There should be an anchor (
<a>) element inside those list items, because if a user clicked on one of those social media names they should be taken into that site. Otherwise having those social media names there would be useless. - That horizontal padding on the list items is huge. Is there a need for much padding? Whenever you are using that big amount of padding, specially in a small component like this, it should be a sign you are doing something wrong.
- The card shouldn't have a
min-width. It should have amax-widthif needed. Because of that massive padding and themin-widthon the card, the card is not responsive in small screen sizes. Ideally a site should look good (responsive without any overflow issues) until around 320px screen sizes. - I would give a small padding to the
bodyso that the card won't hit the edges of the screen.
Marked as helpful2 - Always use
- @sepehrsylvanusSubmitted 3 months agoWhat are you most proud of, and what would you do differently next time?
- Using css variables
- Nice design
- Review HTML CSS
@DarkstarXDDPosted 3 months agoLooks nice. Some suggestions.
- Every page must have a
<main>landmark element wrapping the main content of the page. - The attribution should go inside a
<footer>landmark element, which will be outside of the<main>. - I would use a
<h2>for the user's name here. It acts as a heading for the content that comes after it. - Use
remfor font-sizes. If the user change their browser font size, the text in your site won't react to it because your font-sizes are inpx. If you have the font-sizes inrem, text in your webpage will scale according to the the users preferred browser font size. I would useremformax-widthtoo. - Do you need the
divthat wraps the<p>element which contains the user's bio? I think it's not needed.<p>element alone is fine. Try to keep the HTML simple.
Marked as helpful0 - @CelineJamesSubmitted 3 months agoWhat are you most proud of, and what would you do differently next time?
I am proud of the fact that i am getting comfortable using CSS grid. i am really proud of that, i tried using the mobile first design technique and i prefer that next time i would try to make use of Sass technique.
What challenges did you encounter, and how did you overcome them?Nothing much.
What specific areas of your project would you like help with?well, i am still learning and practicing my efficiency with grid layout.
@DarkstarXDDPosted 3 months agoLooks nice. Some feedback.
- Heading levels should go in order. If you used a
h3for the name, you shouldn't be using ah1for the testimonial text that comes after the name. - You should not have multiple
h1elements in a web page.h1is usually the top most heading that describes the content of the page. In this design you actually don't have ah1. But you can create a visually-hidden element and give that ah1if you want. - Looking at how you have used the heading levels, i feel like you are deciding the heading level based on how they are styled in the design. You should not be doing that. You should decide what elements to use based on their semantic meanings. Not styles. After that you can use CSS to style the headings however you want.
- I would use a
h2for each name and then use<p>elements for the testimonial text. I would also wrap the testimonial<p>elements in a<blockquote>element. - I think it would be better if you give the container a
max-widthinrem, so that the grid items doesn't keep increasing in size on larger screens. - It's better to use
remfor font-sizes, media queries andmax-width. If the user changes their browser font size, text in your site won't respect that if the font-sizes are defined inpx. If you have them inrem, the text will scale accordingly with the user's preferred font size. I usually useremfor all values, not only fonts. But it's just preference.
Marked as helpful0 - Heading levels should go in order. If you used a
- @Amit-R328Submitted 3 months agoWhat specific areas of your project would you like help with?
Any feedback is appreciated
@DarkstarXDDPosted 3 months agoLooks nice. I have couple of feedback.
- The
<header>or thebannerrole is usually used to wrap content that is common across multiple pages of a website. Usually you see it at the top of the webpage wrapping the site logo and a navbar. I would not use a<header>in this challenge. - What you need is a
<main>landmark element. Every web page needs a<main>element wrapping the main content of the page. - The
<nav>element is for a list of links that navigate within the site. Not for links that navigate to outside websites such as social media sites. In this case having the links inside a<ul>is enough.
Marked as helpful1 - The
- @Rgeb1Submitted 3 months agoWhat are you most proud of, and what would you do differently next time?
I'm happy with the result, I don't think I'll do anything differently at this stage, maybe with more practice I'll have better methods.
What challenges did you encounter, and how did you overcome them?Right now my biggest challenge is understanding how to use git and GitHub. I don't use the terminal much, so it's something that I need to start doing a lot more.
@DarkstarXDDPosted 3 months agoLooks good. Some suggestions.
- The
<nav>landmark element is intended for links that navigate within your site, not for external links like the social links. Usually you see the<nav>at the top of the page inside the<header>along with the brand logo. Having the social links inside a<ul>with<li>elements is enough. - I see that you are using
remfor font sizes, padding and margins. Why not useremfor media queries andmax-widthas well? I usually useremfor everything. So if the user changes their browser font size everything will scale accordingly.
Marked as helpful0 - The
- @YuliaLantzbergSubmitted 3 months agoWhat are you most proud of, and what would you do differently next time?
This challenge forced me to learn and understand grid and flex layouts better, as well as the differences and use cases for them.
What challenges did you encounter, and how did you overcome them?First I wanted to use flex layout to build the structure. I didn't find how to make it with flex so I moved to grid. And that made me understand that flex layout is more useful for dynamic and changing designs but grid layout is good for the structure.
@DarkstarXDDPosted 3 months agoLooks good. But i see some foundational mistakes you are doing, that are usually fixed on much more smaller challengers.
- Avoid setting fixed widths and heights on elements containing text. Fixed widths can prevent text containers from resizing properly in smaller screens, leading to overflow issues. Instead use a
max-widthinrem. This will make sure the container won't grow too large, but still will allow them to resize as needed when there is no room in smaller screens. - Avoid using percentage values. They give unpredictable results. One exception would be using something like
width: 100%if you want an element to take up 100% of it's parents width. Other than that I usually useremevery where. - When the screen size shrinks, text wraps into new lines causing the container to increase its height automatically if needed. If you've defined a fixed height, the container can't adjust, leading to text overflow. So don't set a height on text containers. The browser will decide the height based on font size, padding, margins etc.
- So the cards in this project shouldn't have any fixed widths or heights. You only need a
max-widthinremon each card, or on the entire grid. I suggest you go through your QR code component challenge again and remove the fixed widths and heights set there too. - There's no need to change the font size on the html element. Most of the time you never need to change anything on the html element. That's just asking for trouble.
- Start developing mobile first.
- You don't need this many media queries. If you are using this many then that's a sign you are doing something wrong somewhere. Most of the time you will only need 1 media query for the desktop layout, and sometimes 2 if there's a different layout for medium size screens (tablets).
- Avoid using
ids to style. Always use classes to style. In this project, you can give each card a second class name likecard--supervisor,card--calculatoretc. to style individual cards. - I feel like the icons in this design are decorative images which brings no value for screen reader users. For decorative icons you keep the alt attribute empty like this.
alt="". - No need to wrap the
<img>in a<div>. Try to keep the HTML simple as possible.
Marked as helpful0 - Avoid setting fixed widths and heights on elements containing text. Fixed widths can prevent text containers from resizing properly in smaller screens, leading to overflow issues. Instead use a
- @Rapbit27Submitted 3 months agoWhat are you most proud of, and what would you do differently next time?
My understanding of css grid is even better with this challenge. Positioning the cards accordingly was quite satisfying.
What specific areas of your project would you like help with?Please check out my code and give me feedback on how to better setup the grid.
@DarkstarXDDPosted 3 months agoLooks good. Couple of suggestions.
- I don't think the verification status are headings. Definitely not
<h1>s. You should only have one<h1>heading for a page, which will be the primary text that explains the content of the page. It's better to have the user status as<p>elements. - I would always use
remwhen specifying themax-widthand avoid using percentage values formax-width. - Is there a need for the
grid-auto-rows: minmax(10rem, auto)?
Marked as helpful0 - I don't think the verification status are headings. Definitely not
- @roberto-rojas-devSubmitted 3 months agoWhat are you most proud of, and what would you do differently next time?
I loved the style of this card. It was a fun challenge to code.
What challenges did you encounter, and how did you overcome them?I was trying to find a way to set the spacing for all the different elements using fewer lines of code. However, it seems that the distances between some of them are different and unrelated, so I had to set different margin-bottom values for multiple elements.
Is there a better approach for this? Considering that using something like
What specific areas of your project would you like help with?gapormargin-blockwouldn't give the desired result since each distance is slightly different.I would really like to get feedback on my use of semantic HTML tags. I’d also appreciate any advice related to good CSS practices.
Thank you so much for reading. I hope you have an amazing day/night 💜✨
@DarkstarXDDPosted 3 months agoLooks good.
- The spacing between the elements are same in this design. So you can use the gap property without any issues. If somehow the spacing was different, and the difference was like 1px or 2px i would ignore it and still use the gap property. Sometimes designs can have inconsistencies. However, in this design the spacing is same. I checked the Figma file.
- I see you are using values like 16.8px, 15.4px. You most probably never will use broken values like these in actual production. 16.8px is 16px. I assume you don't have access to the Figma file and you may have guessed the values. It's totally fine. Just round the value to the nearest value you feel like.
- The "HTML & CSS" is a heading in this design. It's an
<a>inside a heading. - The text should be in a meaningful element like a paragraph tag
<p>. Text alone inside a<span>or<div>is not recommended since neither of them are meaningful elements. You can replace all thosespantags withptags. - In this design the whole card should be clickable. Not only the "HTML & CSS" part. Try to make it to work. You will need to use a pseudo element on the
<a>for that.
Marked as helpful0 - @AReactDeveloperSubmitted 4 months agoWhat are you most proud of, and what would you do differently next time?
Hello again dear community in this project i used scss preprocessing i didn't wanna use just vanilla like previous one i wanted to learn something new every challenge from this website i tried to get this as pixel perfect as possible though sometimes that gave me a headache considering its very hard to calibrate between margin and padding i know basic box model stuff but I've always struggled with that if you have any tips please leave them down below ill make sure to mark your comment as helpful comment if you care to spare time to review my code help me out with my mistakes and stuff i appreciate that a lot also thank you much for passing by
What challenges did you encounter, and how did you overcome them?trying to get pixel perfect design while calibrating width and height with margin and padding I don't think I overcame that
@DarkstarXDDPosted 4 months agoAs someone has already mentioned it here, I too would suggest you to stop focusing on "pixel perfect". Often, you can only achieve it for very specific screen sizes using bad CSS techniques. Instead what you should focus is developing a website that is accessible and looks good on all screen sizes.
There is a small improvement you can do in your code. You are using
min-widthfor the.profilecontainer, causing it to overflow on screens that are below 370px in width. You can see this if you resize your screen in the browser dev tools. You should develop a solution that looks good on screen sizes down to 320px.min-widthis not needed for a container. What you need is amax-widthso that the container won't keep expanding forever as the screen size increases.0 - @SunilParbhooSubmitted 5 months agoWhat are you most proud of, and what would you do differently next time?
I am proud I was able to complete this project as close to the design as intended. I was glad I was able to closely replicate the desktop, mobile, and active states of the designs. The only part of the challenge I had issues with was changing the font-sizes when going from desktop to mobile, without the use of media queries. I am sure the solution to this is something easy I overlooked, as I attempted using the new CSS feature container-name and @container (max-width ), however I was not successful. Any pointers on that part would be greatly appreciated.
What challenges did you encounter, and how did you overcome them?Aside from the remaining challenge mentioned above regarding the font size changing, I was glad to overcome an issue with resizing the svg image in mobile vs desktop by utilizing the object-fit property.
What specific areas of your project would you like help with?How can I make font-size changes more fluid yet still specific to a size based off a container (without using a media query)? For example, if the card size in desktop has a width of 384px and a paragraph text is 16px, then when the card shrinks to 327px, the text shrinks to 14px?
