Looking to hire developers?
@andreasremdt
@andreasremdt
Submitted
Hey there,
This is my solution to the Huddle landing page with curved sections challenge. I've implemented it with basic HTML and CSS, no frameworks or libraries involved.
The solution is completely responsive and focuses on accessibility.
Let me know what you think :)
@andreasremdt
Posted
PS: I just noticed that the preview screenshot is broken. That might be because I used the latest CSS features, such as color-mix and CSS Nesting which might not be supported yet by the image generator. If you encounter the same issue, you might have to enable experimental feature flags in your browser.
@ricardosilvasx
Submitted
-I don't know if I chose the best names for the classes.
-I would like feedback on the responsive part.
@andreasremdt
Posted
Hey @ricardosilvasx,
Congrats on solving the challenge, well done! Your solution looks good, I like that you wrapped everything in a main tag which is a good practice. Here are some additional suggestions:
h1) and then descend down when it makes sense. In this example, I would make the text Improve your front-end skills by building projects the main heading.div.qrcode, just apply the styles of it to the main element directly.width and height attributes, which I would advice you to set. They allow the browser to better calculate the image dimensions and hence optimize rendering, which results in a better loading experience.Keep up the good work and let me know if you have any questions!
Marked as helpful
@MikeLee0358
Submitted
what is the best way to handle with image and font-size RWD? I tried to limit the container to stop growing up.
@andreasremdt
Posted
Hey @MikeLee0358,
Congrats on finishing the challenge! Let me try and answer your questions:
To limit the width of the container, you can use max-width. I see that you used the clamp() function, which also works but is a little overkill for your needs. Setting max-width: 450px will prevent the container from growing any bigger but still stay flexible on smaller screen sizes.
The image inside the container is already set to width: 100%, which is great. It won't grow any bigger than the container and will shrink with it. I'd suggest adding the width and height attributes in the HTML, so that the browser knows how big the image is. This improves the loading experience, as the space is being "reserved" until the image has loaded. Also, don't forget to set height: auto in your CSS to maintain the correct aspect ratio at all times.
For this challenge, you don't need to change the font size for smaller devices if I remember correctly. So you're good already. Try to set the correct font family though, as this will improve the visuals a lot :-)
Some other things I noticed:
h1 for the heading, as it's the first and only one. Headings in HTML should start from the highest level and descend down if necessary. Since you don't have any other headings, h1 is the best choice here.div#QRCode with a main#QRCode to mark the card as the main landmark on this page. Semantics are an important aspect of HTML development and make life easier for search engines, screen readers, and others. Have a look here for more information.By the way, the method you used to align and center everything (display: grid) is really clever, nice one!
Let me know if you have any questions, keep it up!
@giorgianvatra
Submitted
Hi all,
All the feedback is highly appreciated.
Thank you, Giorgian
@andreasremdt
Posted
Hey Giorgian,
Congrats on finishing this challenge, your solution looks good! I found a few issues that you might want to fix:
form, which makes you lose some important functionality like pressing Enter to submit and send it. This also hurts accessibility.blur), presses the button, or even while typing (input event). So, make sure that you add an else condition in your JavaScript for when the email is valid.a element, as social links tend to redirect you to a different website, whereas buttons are more suitable for triggering interactive JavaScript.I hope my feedback helped you out, let me know if you have any questions. Keep up the great work!
@ricochet69
Submitted
Would a card component like this sit inside a html main tag, or is there something more semantically correct?
I'm not entirely happy with the margin and padding applied to the h1 and p elements. Visually it looks fine but I'm wondering would it have been better to only apply padding on these?
@andreasremdt
Posted
Hey @ricochet69!
Congrats on solving this challenge, the result looks great - it's very close to the design!
Your initial idea of using a main tag to wrap everything is good. To be accessible and semantically correct, every page needs a main landmark, which in this case is the card itself. However, I would alter your HTML structure slightly: you can remove the main.container and apply its styles to the body element. Then, you can change the div.card to main.card. This way, you save on unnecessary elements and make your code clearer while still being semantically correct.
Your margin and padding look good to me, but you could make it even simpler if you applied the padding to div.card-details. Since both the heading and paragraph are direct children, they would be moved into position by that change. I am not sure if you even need this padding, though, might be unnecessary. The margin on both elements looks good.
I am not sure why you went with height: calc(100vh - 1px) on the container? You should be able to use min-height: 100vh without the need for calculations.
Let me know if you have any questions and keep up the good work!
Marked as helpful
@JorgeAMendoza
Submitted
Hello, this is my solution to the dictionary application built with React, TypeScript, SWR, and Styled-Components. End-to-end testing handled with the Cypress testing library.
Any feedback on my project organization, testing practices, and performance would be greatly appreciated.
This project gave me a better understanding on how to use the SWR library to make http request in client components in React. I've used other libraries such as React-query before, so getting experience with other another tool was a great learning experience.
The Axios library is used to make the HTTP request in the fetch function that is called by the `useSWR' hook .
Thank you for viewing my solution, happy coding!
@andreasremdt
Posted
Hey @JorgeAMendoza,
Great job on this challenge! I played around with it and everything works as expected. I especially love the fact that you wrote good E2E test coverage and used ESLint and TypeScript. Your code is very organized and clear, the folder structure and naming are consistent and sensible. I also noticed your attention to accessibility, which is appreciated. So, very well done :)
I found a couple of minor things you might want to fix, but these are only little details:
font-context.tsx, you are using 2 contexts. If you don't think that this is necessary, you can have the font value and setter function on the same context and avoid the overhead.placeholder and aria-label. It's usually an UX anti-pattern to not have a label, but since the design is this way there's nothing us developers can do...Not an issue, just a tip: you implemented your own solution for the font dropdown, which is really good. For the future, I highly recommend using a headless library such as Headless UI or Radix UI. These libraries make it incredibly easy to build such components, because in reality it's quite hard to cover all bases (accessibility, keyboard navigation, edge collision, etc.).
Keep up the great work and let me know if you have any questions :)
Marked as helpful
@andyjv1
Submitted
I had a few issues with the API and the button while building this project. It seems to be fixed now. Any Feedback is welcomed.
@andreasremdt
Posted
Hey @andyjv1,
Nice job finishing this challenge! Since @grace-snow already mentioned some things, I'll purely focus on the React/JavaScript part:
fetchAdvice functions, one in App.jsx and the other in Dicecontainer.jsx. This is duplicated code that you should try and avoid. Ideally, you keep the fetch function inside App.jsx and trigger it with a prop like onClick, which you pass into Dicecontainer. You can then remove the fetchAdvice inside this component and connect the prop with the button's click event.fetchAdvice can be improved. You don't need a timeout to disable/enable the button, this should be depending on the state of the promise. Consider something like this: const fetchAdviceClicked = async () => {
setDisabled(true);
const API_LINK = "https://api.adviceslip.com/advice";
const response = await fetch(API_LINK);
const advice = await response.json();
props.setText(advice.slip)
setDisabled(false);
};
Right before the request is made, you disable the button. Once the request went through (and assuming it was successful), you enable it again.
try/catch: const fetchAdviceClicked = async () => {
try {
setDisabled(true);
const API_LINK = "https://api.adviceslip.com/advice";
const response = await fetch(API_LINK);
const advice = await response.json();
props.setText(advice.slip)
setDisabled(false);
} catch (ex) {
setError("Something went wrong...");
}
};
disabled is truthy, it still has a hover state, which is confusing. Consider adding hover and focus states only when the button is enabled, otherwise, just gray it out. One way to solve this would be by using CSS pseudo classes: button:not(:disabled):hover { ... your code here }App.jsx is an empty array, but the advice slip is an object with properties, such as id. Changing a variable type rarely a good thing, so consider changing the initial state to {} or null.Let me know if you have any questions, I hope my comments are helpful. Keep up the good work and happy coding!
Marked as helpful
@bhoamikhona
Submitted
This is my solution for the Profile card component coding challenge
Links
Built with
Any suggestions for improvement are welcomed.
@andreasremdt
Posted
Hey @bhoamikhona,
Nice job finishing this challenge. I really like how close it is to the design and what you did with the hover effect! Also, your HTML and CSS look good. I only have a couple of minor suggestions:
div.pattern-background, which you used for the pattern background and profile image, has a role of img and an aria-label. I don't think you need this. How I see it, the background pattern is purely for visual purposes and has no importance to screen readers or search engines whatsoever, meaning that you can remove the role and aria-label. Besides, the img element inside has a role of img by default, which would make this markup incorrect as images can't contain other images.<p>
<span class="stat">80K</span>
Followers
</p>
<p>
<span class="stat">803K</span>
Likes
</p>
Using CSS, you can then lay it out properly, for example with Flexbox.
Hope my comments help you, let me know if you have any questions. Happy coding!
Marked as helpful
@2023course
Submitted
Its good to be here my co-developer.
I find it very difficult identifying the color used and also the size of the project. Any recommendation on tools that could make that easy for me on my next task.
Thank You,
Regards.
@andreasremdt
Posted
Hey @2023course,
Congrats on solving this challenge and welcome to Frontend Mentor! Your solution looks good already, it's close to the design and only has some minor issues.
First, you should be able to get the exact colors from the style-guide.md file that's part of the starter code package. It usually contains all colors, font sizes, and more information, depending on the challenge. You are still left to match the correct color to an element, so you either need to be good at guessing or you can use an editing software like Gimp or Photoshop, which contain a color picker. That's what I would do. Same for dimensions, most editing software contains measuring tools.
Secondly, I've got a few suggestions for your solution:
title to the HTML to make it more accessible and SEO friendly.main element instead of a div.h3 for the title, but an h1 would be more appropriate, as it's the only and therefore most important heading of the page. Headings should always start from the highest level and descend down.div.container, you can apply the same CSS rules to your body and remove the container. One element less ;)@import to load fonts is not considered a best practice, as it's render blocking and could lead to decreased performance. Prefer using the <link .../> code from Google Fonts instead.That's about it. Let me know if you have any questions, otherwise, keep up the good work!
@ChrysKoum
Submitted
Any Feedback is Welcome!!
@andreasremdt
Posted
Hey @ChrysKoum!
You did a great job on the challenge, I like that the design looks so close. The hover transitions are nicely done. I found a couple of things that you could improve:
main or section. That's also a complaint on your accessibility report. For example, div.card could be replaced by main.card to indicate that this is the page's main content.h5 for the only page title, while an h1 would be the better choice. Each page should have a main title, and then go lower from there. Since "Equilibrium #3429" is the only title, using an h1 would be much better for accessibility and semantics.a or a button for these elements.div elements to center your card. With just a couple lines of CSS, you can make your life much easier:<body>
<main class="card">
Your card content here...
</main>
</body>
body {
display: grid;
place-items: center;
min-height: 100vh;
}
Hope that these tips help you improve the solution. Let me know if you have any questions :-)
Hi
I have completed another challenge using flexbox. While building the project, I found the box shadow and styling the button difficult.
What is the best resource for learning responsive design? Any suggestions to improve this project are welcome.
@andreasremdt
Posted
Hey @mayor-creator!
Nice job on this challenge, it looks good! Don't worry, responsive design is hard to do right in the beginning, you'll get better. If you like learning from videos, I can highly recommend Kevin Powell's YouTube channel, he explains things about web development in a great way, like this one about responsive design.
I went over your code and can give you a couple of tips:
h2. Since it's the only title on the page, it would be better to use an h1. Each website should start with an h1, since it's the most important title describing the page. If there are more headings, you can lower with h2, h3, and so on.button for "Proceed to Checkout". However, you didn't use interactive elements for the remaining two, which is confusing and hurts UX. Even though they are styled as links, clicking on them doesn't do anything. I recommend using a elements, for this challenge they don't need to point to anything.img.music_icon element for the music icon. In this case, I would leave the alt text empty, because it's mostly for screen reader users. They don't gain any benefit from this icon, it's purely for styling purposes and doesn't add anything to the content. So, you could write alt="" to indicate that the image is not important, or even add aria-label="hidden" to hide it from screen readers.Hope these tips help you out, let me know if you have any questions :-)
Marked as helpful
@mclanecorp
Submitted
I await your comments and criticism, do not hesitate, I ask that I improve myself
@andreasremdt
Posted
Hey @mclanecorp,
Congrats on solving the challenge! I like the fact that you used semantic HTML, considered accessibility, and don't have any HTML issues in your solution. Also, it's not far from the design. A couple of things you could improve:
styles.css, but only use 2 of them. @import statements are expensive performance-wise, so it's advisable to use them as sparingly as possible and without loading resources you don't need.div.container, set a max-width and align everything inside the body using the below code:body {
display grid;
place-items: center;
min-height: 100vh;
em or rem, especially for sizing and spacing. These units are more flexible when it comes to zoomed in viewports and scaling your UI based on font size. This video gives a good explanation of choosing the right units.div, this element can be removed.Hope this helps, let me know if you have any questions :)
Marked as helpful
@mohamedshawgi
Submitted
Your feedback is highly appreciated
@andreasremdt
Posted
Hey @mohamedshawgi,
Your solution looks awesome, close to the design and technically great - no HTML or accessibility issues. I only have a couple of small suggestions:
div elements for each section, you could use section elements. This would make it slightly more semantic, since each section contains its own type of content, but they are still related.p element instead of an h2 for "30-day, hassle-free money back guarantee". Each section should ideally have one heading, followed by content. While having two headings isn't the worst thing to do, it could be confusing because screen reader users might expect more content than there is, since the first sub heading indicates that there are more sub headings further down, which isn't the case.Other than that, you are good! Hope this helps you improving your solution, let me know if you have any questions :-)
Marked as helpful
@sujeong054
Submitted
How do I get the main div in the middle of this whole page? It is located in the middle of the top line, but I want to locate it in the middle of the page. let me know how to do it
@andreasremdt
Posted
Hey @sujeong054,
Nice job on finishing this challenge! It looks really good and you made good use of Flexbox. To answer your question:
body {
display: grid;
place-items: center;
min-height: 100vh;
}
With these three CSS properties, you can center everything within your body to the center. min-height: 100vh is important because it makes the body as big as your browser viewport. Without that line, the body always takes up only the space of its children, so you won't get the effect.
With the above, you can reduce your CSS on the .main element:
.main {
display: flex;
background-color: var(--White);
border-radius: 15px;
height: 650px;
}
Besides that, a couple of additional suggestions:
main element. You can replace the div.main with a main.main to make that work.div and span elements for the text content, which don't convey any semantic meaning. Screen readers and search engines will have trouble understanding what's the product title, its category, or description. I'd recommend the following HTML structure:<p>Perfume</p>
<h1>Gabrielle Essence Eau De Parfum </h1>
<p>A floral, solar and voluptuous interpretation composed by Olivier Polge, Perfumer-Creator for the House of CHANEL.</p>
<div>
<p><span class="sr-only">Current price:</span>$149.99</p>
<p><span class="sr-only">Old price:</span>$169.99</p>
</div>
<button type="button">Add to Card</button>
h1 as the page title and a p for the description. From a perspective of accessibility, the pricing section a bit challenging - while we can visually indicate that the price is reduced, we have to explain it to those who can't see. I included span elements for that, made invisible via the .sr-only class. If you want to learn more about this technique, here's a great explanation.Hope this answers your question and gives you some help. Let me know if you have any questions :-)
Marked as helpful
@VaporDusk
Submitted
How can i improve this project?
@andreasremdt
Posted
Hey @VaporDusk,
Congrats on finishing this challenge! Overall, it's pretty good, your implementation looks very close to the design and works well in browsers. I can give you a couple of suggestions to further improve your code:
header, nav, or similar, but you should wrap your entire structure inside a main element to mark it as the main landmark to browsers and screen readers. You can replace <div class"container"> with <main class="container">.h2 for the first and only heading, but HTML headings should always start at the highest order (h1) and descend down until h6 is reached. Since this challenge only has a single heading, assign it to an h1 to further increase your semantics..container, you can use the body to set the background color and alignment.body {
background-color: hsl(212, 45%, 89%);
display: grid;
place-items: center;
min-height: 100vh;
}
.qr-code h2, .qr-code p. In this challenge, but also in general, it's much easier to set the font family inside the body or html selector, so that it's applied globally and you don't have to target each typography element specifically. Right now, if you were to add another element like a ul, you'd need to update your selector, which becomes annoying quickly.I hope you found these tips helpful, let me know if you have any questions :-)
Marked as helpful
@Toxgem
Submitted
Id like some reviews and best practices for this project
spacing between elements i did with some inline-flex and customs margins but i am sure there are better ways
the sizing of the cards, i've been using rem since it takes root element but i want to know how you would do it.
if there is any visible adjusment that you would make
Thanks in advance for anyone that takes the time to review it
@andreasremdt
Posted
Hey @Toxgem,
Nice work on this challenge! It looks quite close to the design and has no HTML or accessibility issues, which is great!
Here are a couple of possible improvements I can give you:
min-height is used to make the page as big as your browser's window, otherwise the body will only grow as much as the content inside requires it to.body {
display: grid;
place-items: center;
min-height: 100vh;
}
::before and ::after. They allow you to style HTML elements that are not really in your markup, but are still there for you to use. It's a bit tricky to explain in text, so you can check out my solution to see how I solved it.h1 contains a span, which in return has a CSS class for color. You can remove the span and apply the class directly on the h1. The same applies to the below paragraph, which is inside a div without any styling. You can safely remove the div, as it doesn't do anything.rem in most cases. However, I use other units depending on what I am trying to style, e.g. px for border widths. Kevin Powell has a great video explaining when to use which unit, maybe this clears things up for you. https://www.youtube.com/watch?v=N5wpD9Ov_Toimg elements. While this works, it's not optimal in terms of semantic meaning. img elements should be used whenever you want to show an image that's relevant for the content, like a picture of a car on a car sharing site. These two graphics, however, are more for styling purposes and to make the layout look good, so it's better to use CSS background-image for that and remove them from the HTML. If you want to keep using img elements, make sure to apply aria-hidden="true" to them so that screen readers don't think that this is an important piece of content. Also, you can remove the alt text by setting it to alt="". Alt texts should only be present on images that have a meaning to the content.Hope that my tips made things clearer for you, if you have any questions feel free to drop them in the comments :-)
Marked as helpful
@LarBearTow
Submitted
Hey Mentors,
I built this wind nextjs and tailwind. It was much hard than I thought it would be. I'm still learning how to work with responsive designs so I'm sure there is much to improve on.
Any help with responsive layouts would be very helpful. Any other critiques are also more than welcome.
Thanks guys!
@andreasremdt
Posted
Hey @SpiderBear714,
Congrats on solving the challenge! I love Next.js and Tailwind, both are great tools and make an awesome tech stack together.
However, especially in the beginning, I would focus on the basics. While Next.js is a great skill to have, I'd stick to HTML and CSS while solving these simple challenges. Next.js is a fullstack framework that offers so many features, but none of them can be used in this challenge (there's no routing, no client-side logic, no images, no data fetching, etc). All you need is an HTML page and some CSS styles. Adding Next.js, React, JavaScript, Tailwind, and so on just adds a ton of mental and technological overhead you need to get straight in order to get good results, and that's hard for beginners (as well as professionals, I have been writing code for 7+ years and still I am occasionally overwhelmed ;))
So, my tip for you is to focus on HTML and CSS first - get good at it, and then introduce yourself to more of these advanced tools and libraries.
I collected a couple of suggestions to make your code better and cleaner:
title elements, your solution doesn't have one. A good title describes your website or page in very few words, but is incredibly important to your users and search engines. With Next.js, you can add a title like so:<Head>
<title>My page</title>
</Head>
article elements for each of your cards. This would make it slightly more semantic and describe the content better.text-transform: uppercase instead. HTML content rarely needs to be written in uppercase, it's mostly about appearance. The disadvantage of having uppercase words or sentences in your HTML is that search engines index and display it accordingly, which might look unexpected. In Google, you might want your page to read "Sedans", not "SEDANS".border on hover, but without hover there's no border. Borders add to an elements content box, meaning that it grows by however big the border itself is (in your example 2px on each side). That's a common problem, and the simplest solution is to always have a border, but style it properly:<button class="bg-white border-2 border-white hover:bg-transparent></button>
In this example, the border is always there, but it's not really visible due to the background and border having the same color. Only on hover can you see the border, because the background color changes.
focus: to target focus styles, similar to hover:.items-center on the surrounding wrapper. Remove this class and it should look much better. By default, all children inside a flex container stretch to take up the available height, so they should look the same. Setting items-center makes them only take up the space they need and aligns them horizontally.Your responsive styles look good, not much to say about from my side. It's easy with Tailwind to apply responsive styles, though your HTML code tends to suffer in terms of readability from all those classes.
Keep the good work up, things will become easier if you keep writing code :-)
Marked as helpful
@agnar-nomad
Submitted
All feedback appreciated.
I think I finally understand ::after and ::before and I have learnt to do image overlays in this project. It was fairly easy after struggling with the previous tasks.
@andreasremdt
Posted
Hey @agnar-nomad,
Nice job on the challenge! It looks so close to the design, great work! ::before and ::after can be tricky to understand, keep going and these things will become easier. :-)
Some ideas to improve your code:
h3 element, but since it's the first (and only) heading on the page it makes more sense to use an h1 instead. That's also the only remark on your accessibility report. Page headings should always start at the highest (h1) and incrementally go down until the lowest (h6).::before or ::after, it doesn't matter)..nft-container::before {
content: "";
position: absolute;
inset: 0;
background: var(--cyan) url(images/icon-view.svg) center no-repeat;
opacity: 0;
}
The above code achieves the same but is quite simplified:
background-color, background-image, background-position, etc. into a single shorthand property called background.inset: 0 is a shorthand for top: 0; left: 0; bottom: 0; right: 0. It just saves time and lines of code :-)inset: 0, you don't need width and height as the element will stretch all the way.content should be an empty string, since you set the background image via the background property.Additionally, I wouldn't use an ul element for the Ethereum price, since semantically it doesn't make the most sense. A simple div or p should do the job as well and is semantically more correct.
I hope my feedback helped you, let me know if you have any questions :-)
Marked as helpful
@francoisbillet
Submitted
Hi,
Since I want to have good habits and practices from the start, I'd like to ask you a few questions I asked myself when coding this component.
HTML questions:
CSS questions:
@andreasremdt
Posted
Hey @francoisbillet,
Congrats on solving this challenge! It's great that you want to develop good programming habits, you are definitely on the right track :-)
Let me try to answer your questions:
Is it recommended to use a <div> here or a semantic element like <section> ?
Semantic HTML elements always win. As you can see in your code report, you have some accessibility issues because no main landmark has been found. To fix this, replace <div class="component"> with a <main class="component">. Every webpage should have a main landmark, which marks the most important content for search engines, screen readers, and others. There's no need for a section, but for more complex layouts with more content (like blog posts, sidebars, and more), these sure come in handy.
Is there a good practice for naming classes ?
There's no standard, and different people like different naming methodologies. I like to keep my classes simple, e.g. .container, .button-group, .page-title, etc. With CSS, maintainability and good naming are always issues, especially with larger codebases. That's why there are different approaches, like BEM, Atomic CSS, or RSCSS. Have a look at them (and others) and decide for yourself what you like the most, it's your choice.
Is there a good practice for naming custom properties ?
Same as with CSS, it comes down to preference. I like to prefix my variables, for example color variables are called --color-white or --color-primary, while typography variables start with --ff for font family or --fs for font size. But other than that, it's up to you.
How can I center my <div> vertically without adding a manual margin/padding or using flexbox/grid ?
That's pretty easy nowadays thanks to CSS Grid:
body {
display: grid;
place-items: center;
min-height: 100vh;
}
You can remove the margin that you have on the card div and the body itself, they are no longer needed. min-height is needed to make your page as big as your browser is, so that the component is always nicely centered horizontally. Without that line, the component would be stuck at the top.
I'm a little lost with all the available units out there. Which units (px, rem, %, vh/vw, ch, etc.) should I use for which purposes (spacing such as padding/margin or line-height, sizing blocks or images, sizing fonts, etc.) ?
That's a big topic, and there are countless articles, YouTube videos, and comments trying to explain it :D I can only recommend Kevin Powell's video on YouTube, trying to explain which unit to pick: https://www.youtube.com/watch?v=N5wpD9Ov_To His other content is also amazing if you want to learn more about web development.
In this QR code component, what's the most recommended way to handle spacing around the title and paragraph ? Should I use margin on each one (<h1> and <p>) ? Or padding on each one ? Or rather margin (or padding) on the <div> element englobing these 2 elements ?
Whenever you need some distance between elements, margin is the way to go. padding is useful if you want to give your elements breathing room by making them bigger. In this case, I would apply a margin-top and margin-bottom to the heading and try to balance things out. Since there's no content after the paragraph, you don't need to set any margin on it. But be aware that headings and paragraphs have some default margin, so you might have to reset it in order to see the desired effect.
I hope my answers helped you, feel free to reach out if you have further questions. Happy coding! :-)
Marked as helpful
@RayAsh37
Submitted
Hi everyone, I have been trying to practice layout. I am getting pretty confident with it. I have skipped the mobile layout as I plan to come later and implement it for all of my other solutions later. This was a bit simple compared to the ones I previously encountered.
@andreasremdt
Posted
Hey @RayAsh37,
Congrats on solving this challenge! I'd love to see how you tackle the responsive part :-) Your solution looks really good, here are a couple of layout tips I can give you:
mix-blend-mode on the buttons and didn't hard-code the color for each button individually.margin-top: 5rem to push the buttons down. However, this can look weird if the text doesn't have the same length across all three cards. You can observe this issue by resizing your browser's viewport, once you reach smaller screen sizes the buttons won't be aligned horizontally anymore. To fix this, utilize flexbox. On your cards .sub-card, apply display: flex; flex-direction: column; align-items: start;. Then, on the buttons, replace margin-top: 5rem with margin-top: auto. This will push the buttons to the very bottom, no matter how big your screen is.article elements instead of div elements for the cards, to make them more semantic.Hope this feedback helps, let me know if you have any questions :-)
Marked as helpful
@Stv-devl
Submitted
Training APY with advice generator, using Fetch. I also add a green loader.
@andreasremdt
Posted
Hey @Stv-devl,
Great job finishing the challenge! I went through your solution and code and found it working nicely! There are a couple of improvements I can suggest, feel free to implement them or not :-)
.switch-advice is not a real button, but a div element. This hurts your accessibility, because div elements are not meant to be interactive. Whenever a user should be able to click on something, use either a button for programmable actions (like in this case displaying the next advice), or an a element for visiting another page. Right now, screen reader users wouldn't be able to interact with your app, nor would keyboard-only users since the "button" is not focusable.h1 for the heading, but you don't have to write "ADVICE" in all uppercase. It's better to rely on CSS and text-transform: uppercase. Else, if this page was indexed by any search engine, it would display your page title in uppercase, which you might not want.img element with alt="img1" as an attribute. While this works, it can certainly be improved. You could consider using an hr element instead and apply the SVG via background-image, this would be the most semantically correct version. Or, if you want to stick to the img, then it's best set your alt text to alt="" and add aria-hidden="true" as an addtional attribute. The reason being is that screen readers or search engines don't need to care about your separator, it's solely for layout purposes. An image should only have an alt description if it's important content, like the banner image to a blog post. But here, screen readers and other assistive technologies can skip over it.img element with alt="img2", which is not really accessible. First, "img2" is not a descriptive name, especially blind people can only guess what this image is about without a proper description. Second, your button doesn't have any action label, letting users of assistive technologies clueless to what it does. To fix it, you could add title="Display next advice" to the button and add aria-hidden="true" to the image inside. Screen readers will then read "Display next advice" whenever the user interacts with it.I like your JavaScript, it's clean and straight-forward. Also, cool spinner! Hope this helps, let me know if you have any questions :-)
Marked as helpful
@sudhanshu-p
Submitted
@andreasremdt
Posted
Hey @sudhanshu-p,
Congrats on solving the challenge! I played around with it and it works fine, plus it looks really close to the design!
A couple of suggestions to improve your code:
span elements, which have no semantic meaning at all, hence you got 5 accessibility issues. The goal of HTML is to describe the page content to search engines, screen readers, browsers, and other developers as semantically as possible. We have a number of HTML elements for that, such as main, article, section, or headings like h1. If you want to read more about semantics, here's a link to MDN.span, which brings accessibility issues with it (e.g., try to focus and trigger it with your keyboard only - it won't work). Whenever you have interactive content, you should use the appropriate element. In this case, a button is be the best choice, as a button element performs a programmable action after being pressed, like fetching and displaying a new advice. I also saw people use the a element for this, and while it's better than a span, it's also not semantically correct since a elements link to other pages, they don't trigger programmable actions.I like that you used defer on the script import to unblock the HTML from parsing, and I also like how clean and straight-forward your JavaScript logic is. Keep up the good work!
@Darwnsantx
Submitted
@andreasremdt
Posted
Hey @Darwnsantx,
Congrats on solving the challenge! Your solution looks great, it's close to the design and has no semantic or accessibility issues.
I only have a couple of suggestion to improve your code further, but nothing too major:
button element with the type="submit". In this case, you can either remove the attribute or set it to type="button". Submit buttons only work inside form elements, which you don't have here. There's no harm in having it, it's just unnecessary.cursor: pointer to the "Proceed to Payment" button to make it consistent with the other clickable elements. By default, button elements have the standard cursor, changing it to pointer is a common UI pattern and indicates that this element is interactive to the user.img element, which is good. However, to prevent layout shifts while loading your page and its content, you could add a width and height attribute to the element and set it to the width and height of the SVG. It can still be resized via CSS, but by doing this you prevent the elements from jumping around. It's a minor issue and might only ever happen on extremely slow connections, but it's also a good practice when working with images in general.div with class="logo-music" for your music icon. While this works fine, I am a fan of not using HTML elements purely for styling purposes. HTML ideally is used to describe content and not styles. To improve this, you could either use an img element, or (my preferred way) set the background-image property directly to <div class="payment">. Using padding-left, you can then shift the content to the right until it looks like in the design.I hope my tips helped you - if you have any questions, I am happy to answer them :-)
Marked as helpful
@Herteitr
Submitted
I'm not sure if this is the best way to put a hover effect on the picture. I've never liked using the "position : absolute " property. If you know of another way to do this, I'd appreciate it if you could share it.
@andreasremdt
Posted
Hey @Herteitr,
Congrats on finishing this challenge, I like how close it is to the design. Also, your HTML and CSS are really clean and easy to read, nice!
To answer your question about the image: I get what you mean, I also don't like position: absolute that much. However, in these situations, it's the most straight-forward approach and works really good, generally speaking (as long as you don't forget position: relative on the parent ;))
In your solution, you used a div with the eye icon as a child, which is fine. There's a cleaner way with less HTML though, and this can be achieved by using the pseudo elements ::before or ::after.
You can check out my solution, where I've used this approach. It's quite similar to your solution but saves on the HTML markup. Let me know if you have any additional questions :)
Marked as helpful
