@hebaahmedsaleh
What are you most proud of, and what would you do differently next time?
..
What challenges did you encounter, and how did you overcome them?..
What specific areas of your project would you like help with?..
Jye Harry
@jyeharryAll comments
- @jyeharry
Firstly, the
<summary>is hoverable indicating that it should probably be clickable but it's not. Only the plus/minus icon is clickable so it should be the whole summary.I also couldn't trigger the open/close of the accordions using my keyboard only. To make your solution more accessible, address these two issues.
Next major thing is that you've created a custom accordion component which uses JS. But HTML actually now has a native element designed exactly for accordions called
<details>. I would suggest refactoring to use this element.Few minor things:
The background image could probably just be done in CSS rather than as an HTML element on the page.
Personally I wouldn't use the
<h4>and<h6>the way you have, especially the<h6>. It's not a heading so this could be confusing to people with screen readers.Also looks like some styles have been copied over from the interactive-rating-component challenge, so it would be good if they were cleaned out of the codebase.
- @abizmo@jyeharry
Most importantly, the accessibility around the radio buttons could be improved.
For starters, there are no
:focus-visiblestyles meaning when I tab through the page I can't tell whether I've focused onto the inputs.:focus-visiblecan be used to add an outline around the inputs to make it clearer they have focus, which is the case when the submit button is focussed.Secondly, when going over the radio buttons with a screen reader, all that's read out is "Unchecked, Radio button - <num>". This isn't very descriptive to a person using a screen reader especially since the body text doesn't actually explain that they're leaving a rating out of 5.
You're using tailwind which believe has a
.sr-onlyclass which visually hides its content but keeps it in the dom, so that it can be read out by screen readers. So you could do something like<span class='sr-only'>Rating </span>1<span class='sr-only'> out of 5</span>and the screen reader would read out "Unchecked, Radio button - Rating 2 out of 5"A few more more minor things:
There's no need to use opacity on the
You selected <num> out of 5text. It gives it the appearance that the text is 'muted' or disabled.The padding for the card is actually different on large screen vs mobile screen. On large it's 2rem, mobile it's 1.5rem. Try using
clamp()to generate a fluid value that can be used here, it'll save having to use an arbitrary breakpoint. Have a look at Utopia for generating theclamp()value. - @jyeharryWhat are you most proud of, and what would you do differently next time?
I used Astro for the first time which was really nice. What I might do differently is how I style the components. Astros scoped styling is nice but I wonder if having the styles in their own stylesheet is a bit neater and easier to follow since you'd use classnames instead of Astros autogenerated fingerprints.
What challenges did you encounter, and how did you overcome them?Astro encourages using its scoped styling, but due to this when I tried using a separate Astro component in a style selector I found it didn't work despite using
What specific areas of your project would you like help with?:global()in the selector. So I decided to combine the components into the one file again instead of having them in separate files.Probably just the structure of my styles in general but nothing in particular.
@jyeharryDisregard the "00" labels in the popular articles section at the bottom of the design comparison. I think the tool frontendmentor uses to generate the screenshots for the comparison must use an outdated browser or something. I've noticed similar issues on other challenges which used styles that are relatively new.
If you view the live site you'll see the correct label numbers.
- @luissitoe@jyeharry
Some areas for improvement:
-
I think the nav switches to the mobile nav at a point that's unnecessarily wide. It's still at a decently wide tablet screen where the desktop nav has plenty of room. I'm not saying it's bad at all, just that my personal preference is that I'd make the switch happen at a narrower point.
-
Heading hierarchy could be improved. There can be more than one
<h1>on the screen if they're in their own<section>s. Therefore the "New" heading could be a<h1>while the article headings inside that section could be<h2>s. Similarly, the popular article headings could be<h2>s instead of<h3>s. I also used visually hidden<h1>in this section that contained the text "Popular Articles" for accessibility purposes. -
You could simplify the javascript by using a single button that is relatively positioned and has a high z-index, so that when the mobile nav appears the button actually appears above it, allowing you to click it again. The javascipt then gets simplified by only having to add an event listener to one button and in that event listener there's an if statement that determines whether to add or remove a class. You would also then use the applied class to determine whether the button displays the open or close icon. You could look at my solution to understand what I mean.
For my solution I took a lot of inspiration from Kevin Powell's solution, especially for the mobile nav. His solution also taught me some uses for aria attributes which are good for accessibility. I think you could learn a from this video like I did, just try not to copy the code as is and try watching it first then seeing if you can do it on your own.
Marked as helpful -
- @Zxjklp@jyeharry
I think you could benefit from Andy Bells teachings as I have. I've learnt a lot from learning about his css methodology, CubeCSS. It's similar to BEM but allows for even more reuse by coming up with composition classes (layout) and utility classes (think tailwind). Once you've gotten the layout done you can then add in your block classes.
An example in your code of where CubeCSS could make it neater is where you have:
.hero__button:nth-child(2) { background-color: var(--Purple-600); }This style can only be used by one single thing. That is, the second button inside
.hero__button. But changing the background colour of an element could be a pretty common thing. So you move this style into a utility class, something like.bg-purple-600. Now you can apply that class to the second button as well as anywhere that may need it, rather than having to repeat the style in some other elements class.When you start approaching your css in this way, you might find your css becoming neater and smaller as you create more and more reusable classes.
Marked as helpful - @Benson0721What are you most proud of, and what would you do differently next time?
I’ve been working on another grid challenge. To be honest, I don’t think the grid layout itself is too difficult.
What challenges did you encounter, and how did you overcome them?However, I’ve realized that I have some misunderstandings about basic CSS concepts while using grid. This practice requires applying identical color designs to each section, and organizing the CSS has been a challenge for me. I think that’s why I’ve spent almost four hours on it!
What specific areas of your project would you like help with?I believe I have done my best to organize my code structure and address some minor issues. If there are still any mistakes, please feel free to point them out!
@jyeharryYou should look into using BEM class names. You'll be able to make more reusable styles rather than having to target things by id. In real production applications you'd rarely be styling by id, if ever.
Using BEM would help in situations where you're hardcoding data. For example, in
src/Section.jsxyou haveNickname == "daniel" ? (<svg>) : "". So instead of adding all of the data used in these testimonials into an array and iterating over that array to render each card, you could just instantiate each individual card and apply modifier classes that change the appearance of them. So you might have a.section--with-bg-imgclass that applies the quote mark background image, instead of using that ternary operator and checking for "daniel". You could also have a modifier class called.section--span-2which is what makes the section span 2 rows or columns. This way the styles are more reusable instead of relying the exact data that goes in them. The styles shouldn't care about the data.Marked as helpful - @welpmoz@jyeharry
Try to use more descriptive names for your css variables. Also, instead of inlining your css variables to the card components, just give your card components an extra class. Something like
card--redfor the red one. On that note, you should look into BEM class naming conventions. It's a popular way of styling your htmlMarked as helpful - @Francis-Kojo-BuabengWhat are you most proud of, and what would you do differently next time?
I'm proud of the responsive design, ensuring a smooth experience across devices, and the clean layout that highlights the product effectively. The use of CSS variables for a cohesive color scheme adds to the visual appeal.
Next time, I would fix the typo in the font-weight, adopt a mobile-first approach for better scalability, and improve accessibility by adding focus styles for interactive elements.
What specific areas of your project would you like help with?I would appreciate assistance in a few key areas of my project. Firstly, I’m looking for guidance on refining the layout, especially when it comes to ensuring elements are perfectly aligned across different screen sizes. Additionally, I’d love help with making the design more interactive by adding smooth animations or transitions to enhance the user experience. Another area I’m interested in improving is the overall performance—optimizing the code and images to ensure the page loads faster. Lastly, I’d like to explore best practices for structuring the CSS for scalability as the project grows.
@jyeharry- Instead of placing both images in the markup and using media queries to show or hide one of them, follow these docs to implement responsive images using
<picture>. - Look into BEM class naming conventions for naming your css classes. It teaches you to think of your components as encapsulated "blocks" (the B in BEM) and so helps with styling and avoiding class name conflicts.
- On desktop the card should have a max-width which yours has, but on mobile it should probably just stretch to the full width.
- On desktop, the two columns should probably be 50% each rather than the image column being an arbitrary 250px wide and the content column taking up the rest.
- It doesn't really matter, but instead of giving everything IDs you could just give them classes instead. You don't really need to give something an ID unless you specifically need to uniquely identify that thing (such as with javascript).
- It doesn't really make sense to have your
.sub-cardcomponent have a margin only on its top and left sides. Should use margin or padding all around the right column instead. - Lastly, on mobile screens the background is black. It should still be cream coloured.
- Instead of placing both images in the markup and using media queries to show or hide one of them, follow these docs to implement responsive images using
- P@lia-oliveiraWhat are you most proud of, and what would you do differently next time?
Working without Figma was very challenging. I had to manually extract measurements from screenshots. Having access to the Figma design file optimizes workflow and results in better-quality deliverables.
What challenges did you encounter, and how did you overcome them?Relying solely on screenshots is significantly harder. Not having access to precise measurements and font sizes greatly impacts productivity. I will consider this before accepting a freelance project in the future.
What specific areas of your project would you like help with?One issue I noticed was that the title font appeared darker than I expected. I’m not sure why this happened. I believe I followed the style guide's recommendations, but it still turned out differently. If anyone knows what I did wrong, please let me know—I’d really appreciate it."
@jyeharryIn regards to the final look, honestly the only things I can pick out is the fonts being bolder than the design and some spacings around certain elements being a little off but considering you did this without figma, they're pretty minor things.
In terms of your actual html and css, I think there are improvements that could be made.
- You've got a pseudo selector selecting all h2s except for the first one and are applying a border-top to it to get that separator line in the design. Instead of doing this, you could and should use a
<hr>element. - Instead of
<span class='emphasis'>, you could use<strong> - Lastly I'd look into following BEM class naming conventions. It helps to think to think of what you're building as encapsulated elements or "blocks" (the "B" in BEM).
Marked as helpful - You've got a pseudo selector selecting all h2s except for the first one and are applying a border-top to it to get that separator line in the design. Instead of doing this, you could and should use a
- @kiingswayWhat are you most proud of, and what would you do differently next time?
I'm very proud of creating a simple functional layout which is responsive too. I like the layout and the combination of colors. Next time I'll explore the animation use to be as dynamic as possible.
What challenges did you encounter, and how did you overcome them?There was no notable challenges on this project, but the biggest one was working with
What specific areas of your project would you like help with?font-faceand CSS variables that is something I'm not used to.I would like feedback on the accessibility of the card, especially on keyboard navigation and also how to know if my project is looking similar to the final screenshot, I thought the text font was sharper than in the final screenshot, which is obvious, but I felt it was different.
Modern Social Profile Card Design using HTML and CSS
#accessibility#bem#sass/scss#semantic-ui#animation@jyeharryOverall your solution is pretty close to the design however there are some changes you could make.
The provided style guide provides the font-size for paragraphs you could add that to a css variable and use that instead of the arbitrary 18px size variable you've declared. You're also using a font-size of 30px on h1. Consider adding this to it's own variable to make it more reusable. I would also think about making this value smaller too since if you look at the comparison of your solution to the design, this large font-size makes the heading disproportionately large compared to the image above it.
On mobile screen sizes the cards sides touch the edge of the screen. There should be padding on the card container.
To help with accessibility, the links should be tabbable. My guess is it's because they don't have href attributes on them. You can give the href attribute a value of
"#"or"javascript:void(0)"instead of a real url.Lastly, while this is only a small challenge so it probably doesn't matter, I personally like to use BEM class naming conventions on all my css classes rather than using selector combinators like
.card>img. But in this case it's not really big deal.Good job overall.
- @Basselfathy@jyeharry
Personally, rather than having BEM class names that start with "blog", I would've continued using class names that start with "card" as they're still part of the card "block".
Instead of the card being a
<div>, I made mine an<article>to make it more semantic and accessible.Lastly, your media query which adds padding to the
<main>tag only kicks in once the screen reaches 375px, but above that the card briefly touches the edges of the screen. It might be better to just always have that padding on the container.Adding to that, you could avoid using media queries to change the font size by instead using
clamp()which makes the font size change fluidly. :) - @LuisVera1What are you most proud of, and what would you do differently next time?
I am proud because the challenge was not difficult. 👌🏻
@jyeharryYour solution's pretty similar to mine, except you were smart enough to use more semantic tags than I did so good job there.
Personally, the only thing I would change is removing your
.card's fixed height. It doesn't actually matter here, but hypothetically if you were to actually reuse this component but with differing amounts of text, the text may overflow or maybe there'll be too much space in the card due to only a little amount of text.Instead, you could let the content and spacing between that content be what determines the height of the card to allow it to be more fluid.
