Skip to content
OverviewSolutions
27
Comments
509
Fluffy Kas
@FluffyKas

All comments

  • Fluffy Kas•7,655
    @FluffyKas

    Heyo,

    First of all, your solution looks really nice and it works well. The animations you added are great. Maybe it would a nice addition if only one answer could be opened at a time, and all the other opened answers would close when you open a new one.

    Unfortunately, it looks like you forgot to upload your readme file which is a shame, I would've liked to take a look! It's also useful to upload the original version of your CSS and JS, not only the minified version so other developers can take a look at your actual code (minified code is not really for humans).

    From what little I can tell, I would suggest the following small changes:

    • Remove alt tags from your images, there is no need for them (in a sense that the text saying plus or minus icon is in no way helpful to any user to figure out the functionality of the button itself). Your question button already has a text, but maybe it's not quite clear what it does without the visual cue of the +/- icon so perhaps adding an aria-label to the button is a good idea. I'm guessing a bit here, probably someone more well-versed in accessibility could suggest a good solution for this.
    • There are a lot of places where using pixels isn't really great. Most importantly, font-size. You shouldn't make an assumption that your users will use the default browser size of 16px. Using rem here would make sure that your font size scales correctly when those default settings are changed.

    Without looking at the code, I can't really say more, but I think overall you did a great job, well done!

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Heyo,

    This looks great, well done! There is one small issue I noticed: there is an overflow on your header (probably the images causing this). You could just get rid of this with an overflow: hidden on the header, so no big deal.

  • Fluffy Kas•7,655
    @FluffyKas

    Heyo,

    Buttons come with some ugly default styles you need to overwrite, one of these styles is the border. Your problem is being caused by this: border-style: outset. Instead, there is an easier way to overwrite the default look: border: 2px solid white.

    I'd also say, you could adjust the breakpoint a bit, it switches way too early from the mobile to the desktop view, so the desktop view looks really squished. The min-width could easily be around 870px instead of 600.

    The alt texts of the little icons can be left blank, they are decorative only (this goes for most icons).

    Other than these, your solution looks pretty good! Well done.

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Hey there,

    There are a few problems with your code but the main issue that's causing this behaviour is where you placed your preventDefault:

    btnOne.addEventListener('click', function (e) {
  errorMessage.innerHTML = '';
  if (!emailRe.test(email.value)) {
    e.preventDefault();
    errorMessage.innerHTML = 'valid email required';
    email.style.borderColor = 'hsl(4, 100%, 67%)';
    email.style.color = 'hsl(4, 100%, 67%)';
    email.style.backgroundColor = 'hsl(4, 100%, 90%)';
  } else {
    container.classList.add('hidden');
    subscribing.classList.add('show');
  }
});

    So preventDefault will only trigger if the regex test fails. Really, it should be always triggered, no matter the test outcome:

    btnOne.addEventListener('click', function (e) {
  errorMessage.innerHTML = '';
  e.preventDefault();
  if (!emailRe.test(email.value)) {
    errorMessage.innerHTML = 'valid email required';
    email.style.borderColor = 'hsl(4, 100%, 67%)';
    email.style.color = 'hsl(4, 100%, 67%)';
    email.style.backgroundColor = 'hsl(4, 100%, 90%)';
  } else {
    container.classList.add('hidden');
    subscribing.classList.add('show');
  }
});

    You should read the MDN article about preventDefault but basically the problem is the following:

    The default behaviour of any form would be to trigger an action (defined by the action attribute in your html) and make a call to the backend. This action would take a pre-defined route to create, update, etc the database with the data within the form. It's all defined in the backend. Validation checks would also happen in the backend. Depending on what framework you're using, you can catch these validation error messages and display them on the frontend.

    What we're doing in these purely frontend challenges is something different. What we want is to prevent this default behaviour, no matter what, as we have no backend to call. We don't define an action (you can remove the action attr entirely from the html), we just add an eventListener and first we always specify that we don't want the default behaviour (see second code snippet). After this, we can play around however we see fit, implement frontend-only validation checks, apply classes, etc.

    What I described here is obviously a basic scenario, backend isn't always called by the action, there are more "controlled" methods to do that depending on the framework but you'll see that once you try React, Vue, etc.

    Hope the explanation was clear enough, if not ask away.

    There are a few other problems with the code that might cause bugs in the future. I keep it simple because this feedback is getting long already :D

    CSS specificity: I'm sure you've heard of this. We always want to keep specificity as low as possible. Consider the following:

    .information {
  padding: 3rem;
}

 /* INSTEAD OF THIS: */

main .container .information {
  padding: 3rem;
}

    The 2 selectors do the same thing, but the first one has a lower specificity. Higher specificity can lead to unexpected behaviour (like, if you didn't use main .show class but just .show class, it wouldn't overwrite the original CSS rules which had a higher specificity).

    Also, your hidden and show classes don't ever get removed by the JS. If you go back to the main card from the success card, you can see that it has a show AND a hidden class as well. For now this isn't causing bugs, but it could. It's best to add rules to remove them when they are no longer needed.

    Hope this was helpful. Good luck!

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Hey,

    This looks great! Just a few small things you could perhaps do:

    • remove alt text from icons: as they are decorative, they can be left blank
    • you could add some left and right margin to the container - on certain screen sizes (770-920px) it's touching the sides of the screen

    The rest looks awesome, well done!

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Hey,

    Your solution looks good! Seems to work as intended. Folder structure wise, i'm not sure what you're asking about. Vite generated a folder structure for you and this challenge didn't require you to add anything on your own really. One thing, I'd say maybe, don't put the README file in the assets, just overwrite the README generated by Vite - using the README template provided for the challenge by Frontend Mentor - and fill it out (screenshots, thought process, etc).

    The React part of the challenge looks good otherwise. There are only a few accessiblity and markup issues:

    • setting a fixed height on you body isn't ideal. Instead of a width: 100% you could go for a min-width: 100vh to make sure you don't have any height issues on any devices
    • always wrap everything in landmark elements, if nothing else, use at least a <main> as a landmark
    • the divider is decorative, therefor it doesn't require an alt text, you can leave its alt blank (like you did for the other image)

    Question: why did you do this: {JSON.stringify(quote.advice)} instead of just {quote.advice}?

    Overall looks great, so well done (:

  • Fluffy Kas•7,655
    @FluffyKas

    Hey,

    First of all, it looks great. I'm glad to hear you got some useful feedback and was able to tackle this challenge more easily! Some general tips - for this challenge and for the future ones:

    There is a very easy way to place everything in the center - even vertically:

    body {
min-height: 100vh;
display: flex;
align-items: center;
justify-content: center;
}

    Or you can do the same with grid and place-items center, as you prefer. Giving the body a min-height will make sure your card is centered vertically. This way you won't need to rely on %-s for margins. Defining margins with % isn't super reliable anyway, you'd do better with em, rem.

    Getting units right: so apart from margins, generally it's best to use rem when you're uncertain what units you should use - like your card width (where you now use px). Px values shouldn't be used widely, apart from setting smaller things, like a border-radius or box-shadow.

    Alt texts: really nice to see you paid attention to this. Just a note: for decorative images, you can leave alts blank - like the music icon here.

    Semantic stuff: make sure to wrap everything in a main tag. Of course you can have other landmark elements, like header, footer, etc in your websites, but there very least you should use a main.

    All in all, really well done!

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Heyo,

    Nice solution! Usually it's not the best idea to use position absolute to center something that could be done by flexbox or grid. This is going to help you with what you want to achieve with the footer:

    body {
    display: flex;
    justify-content: space-around;
    flex-direction: column;
    align-items: center;
    min-height: 100vh;
}

    If you get rid of the position absolute (and left, top positions), this is going work just fine. Everything else looks nice, well done!

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Hey Karol,

    Looks like a good solution. There are only 2 things I'd mention:

    • For interactions, always choose native interactive elements, inputs elements for inputs, a tags for links, and in this case, a button for making the API call.
    • This is a small thing but in cases like this, instead of giving your card 2 different widths, it might be easier to define just one max-width. This way you could avoid that width-jump at 768px.

    Overall great job though, well done.

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Heyo,

    I think you did a great job here. I couldn't find any weird or unexpected behaviour. This must've been really tough to do. One thing you could add is some sort of visual clue on successful save. Perhaps a little toast notification in the corner that times out after a few seconds. Without any, it's a bit confusing whether I managed to save my changes. But again, hats off to you, this challenge seems like a lot of work.

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Heyo,

    Your solution looks great! I think it's okay to deviate from the original challenge every now and then, I think your background gradient is actually nice.

    As you said, the best solution for this would be a form with radio buttons. It's a bit tricky to style it but possible. Essentially, you'd want to visually hide the original radio buttons and style their labels to look like the numbered buttons. Probably adding a legend as a title ("Ratings") is a good idea too.

    I think the only way to ensure the cards are the same size would be setting their width to be a fixed value which yes, might lead to issues with responsiveness (although you could play around with media queries but it's a lot of hassle for a small problem). I think, what you did here looks great, the two cards, at least to me, seem almost identical so I wouldn't worry about it.

    Overall, even if you didn't choose the most accessible path for this challenge what you did is great. Looks like you put a lot of effort into it, so well done!

  • Fluffy Kas•7,655
    @FluffyKas

    Hey,

    First off, your solution looks great. And it's nice that you made a detailed README, as well. To answer your questions - or more like, I guess to just add my own take on it:

    You will not get a clear answer from ChatGPT, documentation, etc purely because there isn't one best way to write CSS, or code in general. Everyone has their own preferences. In professional settings, there will be company/team guidelines you will need to follow. If you work alone, you can set your own rules. You should ask yourself, how readable your code is. Would you understand it even if you came back to it in a few months?

    But to be a bit more useful, I can give you my own opinion on this.

    CSS vs SCSS

    For small projects, I tend to go for vanilla CSS. Nowadays vanilla CSS is great and it has almost every feature you'd ever need. With the exception of nesting, which doesn't have a great browser support as of yet. If the code is getting longer and I'm not using a framework and my CSS lives in one file, then I go for SCSS. I love it, nesting is easily and improves readability, especially if you follow BEM class naming. +1: If you feel comfortable with CSS, you could look into Tailwind. I'm using Tailwind for most of my projects as of late, and I hugely prefer it over regular CSS or SCSS. If you know what you're doing, it can be an incredibly fast way of styling a page.

    Units

    Eh, this is a tough one. I use em/rem for almost everything (paddings, margins, max-width, etc). For very small things, like border-radius or box-shadow, I use px. vh/vw I use if I want to position stuff in the background - or giving the body a min-height of 100vh. % I honestly rarely use, but I'm sure there are use cases of it. I'd suggest you try a few approaches, but don't stress over this too much. As long as you're using rems and not pixels and not setting a fixed width and height on things, you're mostly good to go.

    Looking at your code what you could do: stop using px for font-size. Rem is the most optimal for this. The height on the body has to be min-height. Margins, paddings you can set in em or rem, vmin... (Kevin Powell has loads of videos on this, with examples).

    Grid&Flexbox

    Again, whatever works for you. In this regard, as long as it looks good, there isn't really a bad way to do it or such things as too much flexbox.

    Ok, I think I wrote enough. :)) As I mentioned, if you don't already know Kevin Powell's channel, do check it out, he has a video on everything that is CSS related, probably. You did great, I'd suggest not to stress too much and just practice. :) You'll get a feel to it.

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Heyo,

    Your solution looks really good. I noticed a few small things you could improve:

    • Double-check the hover color on the links and buttons, I'm assuming that green colour isn't in the design originally?
    • For small transition animations, like the ones you use in the navbar 0.5s is really long. 0.2s or 0.25s is usually more pleasing to the eye.

    Your markup could be more semantic:

    • You're using a div where you should be using buttons (the div with arrows). Alt texts for these arrows should also be left blank, instead, you could add an aria-label to the buttons with a helper text like "Left" and "Right". Similar goes for the Shop Now button, you should use a link not a p tag.
    • The whole page should be wrapped in a main container instead of a section.
    • Your nav links should be living in a <nav>, and the list of links should be a <ul> with links inside wrapped in <li> tags.
    • There should be a h1 on every page (kinda like a main page title), in this design it would make sense if the "Discover innovative ways to decorate" would be that one.
    • Not every image has to live in a figure. I'd really only add a figure if I wanted to add a caption to my image. There might be other use cases but tbh I personally never had to use it before for anything else than adding caption.

    You got the looks right, so well done! It's a good idea though to stop sometimes and think about the function of each element on the webpage you're developing. Getting a semantic markup right is just as important as the looks.

  • Fluffy Kas•7,655
    @FluffyKas

    Heyo,

    Looks like a really good solution! One thing you could do is to disable the submit button until the user chooses a rating to prevent empty submissions. Few tips to improve on accessibility:

    • They best solution for this sort of rating system is radio buttons. The function of a list of regular buttons may not be immediately obvious for someone who doesn't rely on visuals to use the internet (e.g. ppl using screen readers). To achieve this, you can visually hide the radio buttons and display their labels as the clickable "buttons". Plus, this makes handling the JS bit much easier too. Probably you won't rework the challenge, but I thought it's something you might wanna keep in mind the next time you face a problem like this.
    • Although this is just a component, it's generally a good idea to wrap all your content in at least a <main> tag.
    • For decorative images, you should leave the alt text blank (the star icon and the little image on the thank you page could be both considered decorative images).

    Well done :)

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Heyo,

    Looks pretty good! JS wise the only thing I'd double-check is the regex. test@test passed as a valid email address even though it's not exactly valid.

    Semantic markup:

    • You should wrap everything in a <main>. This could be instead of the div called main-wrapper. Alternatively, you can separate the content of the page into header (ping logo), main (form and image) and footer (socials).
    • The social links should be actual links with aria-labels describing where they lead. Best to wrap these in a <ul>.
    • The input and button should be wrapped in a <form>. A label is also missing. Each input should have a label describing what they do, a placeholder is not a reliable equivalent of this. Since the design doesn't contain the label itself, it can be easily hidden with an sr-only (for screen readers) class, like so:
    .sr-only {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip: rect(0, 0, 0, 0);
white-space: nowrap;
border-width: 0;
}

    CSS:

    • The mobile and desktop sizes are good, but the tablet size is a bit lacking. You could give a max-width to the image and to the form as well so they don't stretch the whole width of the screen.
    • The input has a huge left padding in desktop view, you could perhaps reduce this a bit to look nicer.
    • Socials: I saw this code there:
    @media (min-width: 1440px)
.social-wrapper {
    padding: 0rem 40.9375rem;
}
.social-wrapper {
    display: grid;
    grid-template-columns: repeat(3, 1fr);
    grid-template-rows: 1fr;
    padding: 0rem 7.8125rem;
}

    I see what you're trying to achieve here but instead of the huge padding, you could do something easier:

    display: flex;
justify-content: center;
gap: 1.5rem; (note that this is just a guess)

    This would be a bit harder to achieve with a grid, at least how you have done it. With flexbox you probably won't even need a media query.

    I hope I was able to help! (:

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Hey,

    Your solution looks great! I'm not a 100% sure what's causing the problem with the screenshot. If I had to take a guess it has to do something with the font-size: inherit. I changed the font size to 1rem and that way I was able to recreate the weird behaviour that you can see on the screenshot. If you leave it on inherit, the computed value is only 15px, so that's 1px difference. Maybe you could try to set the font-size in your code explicitly to 0.9375rem. The problem didn't show up on my end so I'm not a 100% sure but that's all I could think of now.

    Some other things to note:

    • Your website your should only have one h1. This is like a main title for a webpage. In this case, since it's just a component you probably don't even need a h1, these headings could be h2 or h3.
    • Alt texts are displayed in lieu of an image but if the image only serves decorative purposes (like these icons do), you should leave the alt texts blank, so screen readers can skip it.

    Everything else seems great to me, responsiveness is really nice too. Well done!

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Heyo,

    So there's a few things to cover here:

    1. Adding a folder: this is something you need to do in your code editor and not in Github. Usually you can just right-click on the project's folder to add a new folder or file inside.
    2. You image doesn't show up in the live site because the source path you defined is slightly incorrect: it should be "./image-qr-code.png" instead of "/image-qr-code.png"
    3. If you'd like to use a certain font family, you first you need to import it (which you did) and then apply it (this you forgot). Usually you can apply this to the body itself unless the challenge has multiple font families. Google Fonts even gives you a code snippet for this, but it should be something like this: font-family: Outfit, sans-serif.
    4. The alt text of images should be human-readable. This is what gets displayed when your image doesn't show up or if someone's using a screen reader. Since the challenge is in English, it should be like this: "QR code" or "Frontend Mentor's QR code".
    5. You can add a custom background but if you add an image, download it and add it to your images folder, the performance might be better that way. The effect you're going for here can be achieved through other means, it looks like a gradient. You can read more on how to do this on MDN.
    6. For the body, instead of a fixed height, it's better to use a min-height: 100vh, it's more responsive that way.

    I hope this helped you a bit. Good luck!

    Marked as helpful
  • Fluffy Kas•7,655
    @FluffyKas

    Hello,

    Very neat solution! Kate has already answered your question, so I'm just gonna mention something else: your social links would be better off semantically as a <ul> with list items. Each image could be wrapped in a link with the appropriate aria-label to describe where they are leading to (for people who don't rely on visuals, but use screen readers for example). Everything else seems great, well done!

    Marked as helpful