Skip to content
OverviewSolutions
27
Comments
26
Sarah
@sarahc-dev

All comments

  • Sarah•835
    @sarahc-devPosted over 2 years ago

    This looks great!

    I love the image zoom animation at the bottom! Really nice touch.

    1
  • Niti Mittal•260
    @NitiMittalSubmitted almost 3 years ago

    I am having issue while making my page responsive for exercise card. It will be good if someone can have alook over my code and let me know where I am doing wrong.

    Responsive time tracking dashboard

    1
    Sarah•835
    @sarahc-devPosted almost 3 years ago

    Hey!

    I think the problem is you are using % for the width of your cards - so they grow and shrink with the screen and the flex-wrap can never kick in. If you change you cards to a width or max-width in rem, the flex wrap will then work as it should.

    Also - if you want to use flexbox and not grid - I would create a separate container for the time tracking cards - and apply flex only to these (and not the initial profile card). Then you don't have the issue of trying to move the second row across.

    Hope that helps :)

    2
  • rubbersouls•180
    @rubbersoulsSubmitted almost 3 years ago

    This is my first exercise done with HTML and CSS from frontendmentor.io The code is not perfect but it more or less corresponds to the model :D And it is responsive If you have any advice to improve my code I'm a taker, thank you

    Order summary component by Rubbersouls

    3
    Sarah•835
    @sarahc-devPosted almost 3 years ago

    Hey! This looks good - so well done! A few tips that might help you on future challenges -

    • Use min-height: 100vh instead of 100% on the body and don't set a height on the html tag.
    • You don't need so many divs! If it only contains one element - you probably don't need it.
    • Avoid setting a height and width on everything - try to use padding and margin to create the spacing instead.
    • For accessibility, avoid using px values for font sizes, heights and widths - use rem or em instead

    Hope that helps. Keep going!

    Marked as helpful
    1
  • Sarah•835
    @sarahc-devPosted almost 3 years ago

    This looks great - congrats!

    Just one tip - you should write your code mobile first and use a media query with min-width to make the amendments for larger screen sizes. It's best to get used to this way from the beginning :)

    Hope that helps!

    Marked as helpful
    1
  • Archie•30
    @archie-kSubmitted almost 3 years ago

    Hey guys, please let me know where i should improve my css spaghetti code, any criticism and tips are welcome. Thanks you so much

    3 column card component

    1
    Sarah•835
    @sarahc-devPosted almost 3 years ago

    Hey! This looks good - I wouldn't say you have spaghetti code at all!

    • Using h1 instead of h2 in this instance would stop the accessibility report shouting at you - you wouldn't typically have more than one h1 - but in this design where the 3 main headers are of equal importance it doesn't cause a html error I don't believe.
    • Use rem for your font sizes instead of px for better accessibility (allows a user to set browser to a larger font). But if you do this, you also largely need to set heights and widths if using in rem or em so everything's container can also grow if font is set to large.
    • I wouldn't use justify-content: space-around to create the spacing between the text - i have a thin screen and everything gets squashed for me. Better to use padding or margins to ensure the space is always there on different screen sizes.

    Hope that helps :)

    0
  • Jessica•880
    @perezjprz19Submitted almost 3 years ago

    A question that came while I was starting to build this, is:

    • If all sections have the same importance, how do you determine what heading to use since having multiple H1 headings is frowned upon, but in context, it doesn't make sense to have more than one level of heading or have a different type of tag that's styled the same as a heading?

    • I felt like using margin at the bottom of the paragraph to create the spacing was a bit of hack. Is there a better way to do that?

    • I wanted the buttons to remain in the same place regardless of the space taken up by the paragraph. Is there a way to achieve that without using absolute positioning?

    • Any tips to improve the legibility of my code would definitely be helpful.

    Responsive 3-column layout using grid and flexbox

    2
    Sarah•835
    @sarahc-devPosted almost 3 years ago

    Hey!

    • My understanding is that if you are using semantic HTML like <article> or <section> and the heading for each is of equal importance such as in this design and there is no other main heading - it is ok to use multiple h1s.
    • Using margin under the paragraph is what is giving you the height of the container. An alternative would be to set a height in rem or vh and use margin-top:auto on the button to push it to the bottom. You have to be careful of the text overflowing at different screen sizes if you do this - but it does keep the button in the same place.

    Hope that helps!

    Marked as helpful
    1
  • Sarah•835
    @sarahc-devPosted almost 3 years ago

    This looks great! The only thing you could have done was added a media query to increase the spacing at the top on large screen sizes :)

    1
  • Sarah•835
    @sarahc-devPosted almost 3 years ago

    Hey! This is looking good. Just a couple of suggestions to help you - you don't need to use a media query for the mobile size. That should be the default, and only use a media query for larger screens. You have a weird situation where you lose the background and the card grows too big between 375px and the large screen media query. You can use a max-width on the card to prevent it growing too much before the media query kicks in.

    Also add some blur to the box shadow to make it less harsh e.g box-shadow: 0px 20px 8px hsl(228deg 45% 96%);.

    Hope that helps :)

    Marked as helpful
    0
  • Igor Ostojic•300
    @igor-ostojicSubmitted almost 3 years ago

    I'm proud of the work I've done on this mini app, but I have a question.

    Was there an easier way of changing the data when a button is clicked ? I feel i may have over complicated things.

    Thanks in advance !

    Time Tracking Dashboard App

    1
    Sarah•835
    @sarahc-devPosted almost 3 years ago

    Hey!

    I also completed this challenge today. I have used one forEach() to loop through the JSON file and then used an if statement to update with the relevant data - rather than using a for loop on every element. Have a look at my solution, you may think it is an easier way (ignoring the additional complication of trying to animate it!)?

    I like the way you have rendered the whole component using Javascript to make it more dynamic - I didn't think of doing that!

    1
  • Sarah•835
    @sarahc-devPosted over 3 years ago

    Hi

    I would suggest not using a set vh or vw on your main and your text. I have a small macbook air screen and it cuts off most of your text because you've used a fixed height and it doesn't all fit on my narrow screen size. Use padding on the body instead to add the border area around the main, and allow the main content to grow as needed to suit each screen. You can use min-height, but not setting a fixed height on things should really help you with responsiveness.

    Hope that helps :)

    2
  • Alain Moratalla•135
    @almoratallaSubmitted over 3 years ago

    Hey there! I am open to any suggestions or feedbacks. Please try this game as well if you like. I've also added a semi hard mode where it will try to guess your choices based from your previous choice pattern. Feel free to share any tips or suggestions. Any form of remarks will be highly appreciated. Thank you!

    Responsive, SCSS, Mobile-first, VanillaJS and Brain.js RPSLS game

    2
    Sarah•835
    @sarahc-devPosted over 3 years ago

    Wow, this is great! Love it :)

    1
  • Sarah•835
    @sarahc-devPosted over 3 years ago

    Use border-radius :)

    0