Skip to content
OverviewSolutions
27
Comments
510

Fluffy Kas

@FluffyKasHungary7,675 points

I'm a web developer. CSS is my passion. I'm most comfortable with a React-Typescript setup, but I try out new things sometimes. When I'm not working, you can find me at the gym or playing games at my shiny PC.

I’m currently learning...

I'm learning more about the Other Side - backend as some people call it (Node.js, NestJS). Occasionally get stuck with git but I'm learning more, so one day we can be friends.

Latest solutions

View more solutions

Latest comments

  • Fluffy Kas•7,675
    @FluffyKasPosted over 1 year ago

    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
    0
  • Fluffy Kas•7,675
    @FluffyKasPosted over 1 year ago

    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.

    1
  • Fluffy Kas•7,675
    @FluffyKasPosted over 1 year ago

    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
    1
  • Fluffy Kas•7,675
    @FluffyKasPosted over 1 year ago

    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
    2
  • Fluffy Kas•7,675
    @FluffyKasPosted over 1 year ago

    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
    2
  • Fluffy Kas•7,675
    @FluffyKasPosted over 1 year ago

    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 (:

    0
View more comments