Skip to content
OverviewSolutions
5
Comments
32
Joran Minjon
@DrKlonk

All comments

  • Joran Minjon•610
    @DrKlonk

    Hi Luka, nice to see you on the platform!

    Some (unordered) things that stick out to me:

    The centering doesn't really work on smaller screens, as Simone pointed out. Use margin auto, flexbox or grid for this to keep it nice and responsive.

    It seems like the responsive.css is the same as style.css. No need to repeat everything in a media query, just the things that are changing.

    The border radius of the card seems a bit off, I think it can be just one value.

    Using rem instead of px is usually preferred, because it increases responsiveness and thereby accessibility.

    Naming classes is hard, but can definitely be improved here. Try to come up with something that describes the content of the element. "Annual-plan" is a decent name in that sense, "div-button" is not. You might want to call that "button-group". Also "rand-text" suggests the text to be random, which it is not.

    Naming things is super important, even more so when collaborating with others.

    The annual-plan class can be centered more cleanly by removing max-width and left properties and introducing margin: 0 2rem or something similar

    Marked as helpful
  • Joran Minjon•610
    @DrKlonk

    Hi Maja,

    I agree with the others that CSS Flexbox is a great way to accomplish flexible layouts. I'd like to add Flexbox Froggy as an additional resource to practice with it!

    Happy coding!

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Hi Agata,

    Nice solution! Good to read (in your readme) that using a framework would do nicely here, especially with the reusable components. I fully agree! It is well worth picking up one of them.

    I especially like the visually hidden headers on sections for our visually impaired visitors.

    Well done and happy coding! Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Hi Jeremy,

    Nice job overall! Some things I've noticed:

    The third rating box should be: justify-content: flex-end (instead of end).

    The stars wrap from 640-653 pixels wide for the middle rating, that looks a bit weird. Also, from 792 to 426 pixels wide the stars seem to move upon resizing. They don't align properly with the other ratings either.

    All this is because the rating__stars resizes because of the flex-shrink, but the stars have a fixed width. I think it is better to remove the flex-shrink from this class.

    Btw, flex: 0 1 auto is the default, so you don't even have to set the growth and shrink explicitly!

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Hi Raymart!

    Reaaaaaally well done on this one. Looks good, works good. A nice example for everyone to follow.

    Particularly, I like the little animation on the 'rules' text and the fact that you don't get points reducted when you lose.

    A minor thing I found in the scss: I think technically the flex classes like flex--j-between are utility classes and not variables. I'd expect a _variables.scss to contain only scss variables. But maybe that's just me. You could check out the 7-1 pattern to structure SCSS. I really like it.

    Just one minor thing I found in the code: in displayResult() the winner always gets set to playerChoice, although I don't think it affects anything.

    Again, very well done!

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Hi Zach!

    Nice job for your first challenge! Don't be misled by the word newbie for this one, it is quite tricky. The desktop version looks nice!

    Main thing: responsiveness I see some problems when I resize the screen to a mobile width, it kind of breaks. It works at 376px and lower, but everything in between is a bit wonky. If I resize the window, I get horizontal scrolling. And if I resize within devtools, at 775px for instance, the paragraph text size is too small because of the automatic resizing. On tablet sizes, the background image get pushed to the left, eventually going past the left side of hero__cta and the screen.

    I think these issues are caused by the grid that can't fit the screen.

    Random other things

    • You can add some line-height to the paragraph to gice it some space between lines.
    • There are no error messages for the email input.
    • It would also be nice if the google podcasts logo was centered vertically. Nothing a little flexbox can't fix.

    If you like, you can check out my solution for this challenge as well, it can at least help if you want to make the error messages.

    Don't be discouraged by my comments, you already took a very good step to join the community. Just trying to help!

    Keep on coding! You'll just get better and better.

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Hi Saurabh,

    Decent work! The site resizes properly and I like that there is some feedback on clicking buttons (even though it is not that meaningful).

    Some things to consider: Meta:

    • Please use a separate file for your css. It is bad practice to put everything in one file. You can import the css file in your <head>.
    • The class names are not consistent enough. Sometimes you do use the elements name (section/span/etc), sometimes you don't. Try to make the class names represent what the CSS is about. If the CSS is specific for the footer, call it 'footer'. If it is more of a utility class that gets reused, call it 'info-text-left' or something like that. Reusing a class named 'grow-together' for the same thing is a bit weird.
    • Do fix the html and accessibility errors generated by Frontend Mentor
    • I always like a cursor: pointer on buttons.

    Design:

    • The text on nearly all buttons is quite tiny. Go for at least 14px (or 0.875rem).
    • The top section needs more breathing room. It is quite different from the design in font size and margin of the text.
    • The big image with the colored dots looks stretched in the vertical direction. You can just remove the height property to fix this.
    • The headers from "Grow together" and such are also too small. Compare it with the design and see the difference.
    • You should hardly ever text-align: center a paragraph of text. It makes it hard to read. Check the design here as well, it is different.
    • The "ready to build" part actually has a bit too much space in my opinion.
    • The wavy image near the bottom has some whitespace on its bottom on some screen widths for me in Chrome. I can't find out why, but it shouldn't be there. It might be because in the HTML the <img> is just floating there in between the main and the footer. You could make it the background of a before pseudo element of the footer, maybe that helps. Removing the width: 100% also removes the whitespace, but the result might not be what you want.
    • In the footer I'd expect the subscribe button to also trigger the alert but it didn't.

    A lot of stuff! It's not all as important, but definitely enough room to improve.

    Happy coding!

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Hi Vitor,

    I like it! There are some minor things to improve, but overall it works nicely.

    I somehow didn't even know that creating the <input> inside of the <label> element was a thing. Mozilla shows it as an alternative, so it seems like a completely valid option. I asked a question in Slack#best-practices as to what is the preferred method.

    You can also toggle a class on an element in javascript with element.classList.toggle('.className'). I think that would lead to some cleaner code in your solution.

    However, I think I prefer to handle the dark theme switch method described here. That saves adding classes to all items and writing those classes separately in the CSS.

    In the smaller cards all the arrows are green and point upwards. Easy fix!

    All in all, a great job!

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Hi Vinicius,

    I like it a lot! The responsiveness is on point and it all looks nice.

    Some minor details:

    • On smaller screens, the headings of the cards could use some room to breathe on the top.
    • The paragraphs of text in the cards should be <p> instead of <span>.
    • The classless div inside of the get-started div could be removed. It also lacks padding when resizing (check 346px for instance)
    • The links in the footer should semantically be a list, I think
    • You have some purposefully empty columns in the footer grid, for spacing. I think that is not that nice. Also, on 983px wide, the column for the first set of links is much smaller than for the second. I would not expect that. I think that extra space is for positioning the social icons properly, but that could be done without the empty columns too. You could also position the items inside the social column differently to fix this.
    • The <picture> tag is mainly used for multiple responsive images. I don't think it is needed here.

    Again, it may look like a lot, but they are all quite minor and the page works as it should.

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Hi AbdulKareem,

    I would also check out the accessibility issues in the generated report. Those are quite relevant.

    Design wise, the biggest thing for me would be the font and the way the text is styled. Check out here how to add a font to your page.

    The padding on the paragraph makes it off line with the heading, which looks strange. I'd just remove the padding.

    Also, your background is a bit darker than the design, making for less contrast on the paragraph, which makes it a bit more difficult to read.

    In the code you use px and em for sizes a lot, I think it's well worth looking into using rems for all your sizes. It allows you to resize basically your entire site by setting the font-size in a media query. Check out this article on px vs rem, or do some googling on it.

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Looks good overall!

    In the share button I see some room for improvement.

    It would be nice to have it disable if you click outside of it as well.

    It is also a bit weird that on smaller screens it covers the name and date (but maybe that was in the design).

    In the code it is not great to have it dependent on the color of the button. I would probably toggle a class (button.classList.toggle('active')) on the button on click and fix everything in the CSS based on if the button has that class or not.

    You could select the icon-social element in CSS with the "~" selector I think.

    Also, it looks like you are tyring to use BEM for naming classes, but it's not consistent. If title__content is an element of the content block, it should be content__title. After that you also use contact-user and userInfo, which are a bit ambiguous and use a different structure (hyphen and camelCase). Naming classes can definitely be tough, but can lead to much easier to read code. So it's well worth looking into! It's one of my own points of improvement as well.

    Cheers, Joran

    Marked as helpful
  • Joran Minjon•610
    @DrKlonk

    Looks good to me!

    What do you mean with the extra space below the text? Which text(s) do you mean?

    I think the buttons look a bit off centre because of 1) the font-family and 2) the fact that these words have little letters going down of the baseline (like g, p, or y for instance), making it a bit more obvious. It's a bit harder to get annoyed with the pixels in "Python" than it is in "Frontend", in my opinion.

    Here's my font-family proof: If you change the font to monospace, the clickable labels look fine.

    Fix for the smaller labels: On the labels like "NEW!" you could just add some more padding to the top in the job__feat class. I don't see much problem with that, except when you decide to change the complete font family, in which case you'd need to adapt at most 2 lines of CSS. No biggy.

    Minor improvement: I'd also add a good old cursor: pointer; to the job__position class.

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Hi Mayokun, there are some other issues I found (after some url hacking) as well. Most of them easy fixes!

    • I'd remove the transition on the main component, it is moving slowly on resizing. That looks weird.
    • The rules box is the same color as the score box, making it hard to see the border in the top right part. Some box-shadow would do nicely, I think
    • from 376px to ~585px wide, the score card is out of frame, while it is in frame for smaller and bigger sizes. You could just use the same process to make this fit
    • If the screen is super wide, the Rules-button flies away a bit. Try a max-width on the body to keep it close to the game.
    • Don't use a <button> inside an <a> tag, that is semantically incorrect. They can both be used to provide links to stuff. My default is <a> for (external) links, <button> for actions on the same page. So here, an anchor with styling to make it look like a button would suffice.
    • There is still some console.logging done when clicking the buttons
    • I dont like the red text inside the "play again" button. It makes it look like I lose all the time ;)

    The game works fine though! That's the most important. Good job.

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    You should remove the "/picked/scissors" from the live url. Now, this doesn't work out of the box.

  • Joran Minjon•610
    @DrKlonk

    Hi Sebastian,

    Here are some things I noticed:

    • The right gradient is actually on top of the mobile phone on smaller screens (it is not a background image in that sense)
    • There is a "filler" div which does not seem to do anything
    • There is a gradient div in the left div which should be removed (it is a duplicate which shows on really wide screens)

    A nicer way to fix this, in my opinion, would be to use the before and after pseudo element on the body to position the purple things. Check out the way @grace-snow did it here

    On the use of CSS grid:

    • You are overriding the areas with the columns in the middle div.
    • The columns are a bit strange. You are kind of building a 3 column layout with 12 columns. Why not use it like so? grid-template-columns: "4fr 5fr 3fr"; grid-template-areas: "l m r"; But in the end, I think CSS grid is a bit much.

    I think using a flebox with just the phone and the text elements in it would suffice for this challenge.

    Also, try to use more semantic elements like <main> and give your classes more meaningful names, for instance "introduction" instead of "text".

    Keep on happy coding!

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Hi Otman,

    I was browsing through the solutions for this challenge and went back a long time to see how others fixed it.

    So this advice is a bit late, but better late than never.

    • The green dots are continously moving. Is that on purpose? I find it quite distracting.
    • You can import the svgs just like you normally would in the <img> tag, in the src attribute. That way, you can keep your html a bit cleaner.
    • You could make the podcast logos a bit nicer by not giving them height and width of 100%, but creating a lexbox and aligning them in the center. Then the google podcasts logo wouldn't look stretched. Because now, they all get the same height. Fiddling around with the margin is not the way to go.
    • The error message is in a different font (Times) and the input field as well (Arial). That is a bit strange, you can just use the same as the text. I think that would be nicer.

    All in all, nice job! The responsiveness works well.

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Hi Agata,

    I "fixed" the SVG problem in my solution of this challenge. I put two svg filters inside the html so I can reference them in CSS. It is very ugly though and I did not find a way to put them in an external file and make it work. That would be an ideal solution.

    I think you're right that changing the fill does not work when there is an feColorMatrix in play. So you have to change the filter to change the color, or change the SVG altogether and wrestle out the color matrix.

    Cheers, Joran

  • Joran Minjon•610
    @DrKlonk

    Hi Milo,

    On a quick glance, this looks like an excellent opportunity to use the <meter> element. I must say I've never used it, haha. I just know of the existence for a short time.

    Cheers, Joran