Skip to content
OverviewSolutions
9
Comments
28
Shalom2935
@Shalom2935

All comments

  • Shalom2935•420
    @Shalom2935Posted over 1 year ago

    you must use semantics markups to make your site more accessible.

    Marked as helpful
    0
  • Shalom2935•420
    @Shalom2935Posted over 1 year ago

    hello well done completing this challenge. I think those tips might help you improve your code:

    • this is the magic formula whenever you want to center a div whether it is horizontally or vertically:
    body {
display: flex;
justify-content: center;
align-items: center;
}

    the margin you gave will prevent the site from being responsive.

    • giving your main element a max-width is great but it is for no use if you don't add a width property.
    main{
width:80%;
max-width:416px;
}

    Hope my comment will be useful, happy coding.

    0
  • Dan McGrath•10
    @Dan-McGrathSubmitted over 1 year ago

    Questions on images

    Pretty simple overall, but I was having difficulty getting the image in the spot I wanted. It kept wanting to overflow its container. The only solution I found was setting theimg {width: 97%}. Why that number? I'm not sure when I set the width to 100% the image becomes offset. If you know why this is, could you please let me know?

    Update

    Made changes to HTML and SCSS files. Let me know if there's any other changes I could make. Thank you For the help!

    QR Component using SCSS

    3
    Shalom2935•420
    @Shalom2935Posted over 1 year ago

    100% didn't work because you gave your image a margin. Whatever this is a better way to make an image responsive.

    1. give the image a 100% width and DON'T SET ITS HEIGHT!!!
    2. give its container the wanted width and a height that adjust automatically (auto) in your case you should have something like this:
    .qr-img {
width:100%;
height:auto;
}
.qr-img img{
width:100%;
}
    Marked as helpful
    1
  • Shalom2935•420
    @Shalom2935Posted over 1 year ago

    Great job. I like your minimal file's structure. Regarding your respond mixin, this is a tip that will help you have a shorter and very cleaner code.

    • To begin with, don't name your breakpoints after the device's name but use something more general. Nowadays, phones can be bigger than tablets and tablets can be bigger than some tinny computers so it could be very confusing. you can have:
    1. desktop -> large
    2. tablet -> medium
    3. phone -> small
    4. small-phone -> smaller
    5. vs-phone -> verysmall
    • Then you can use the following code instead of all those conditions:
    $breakpoints: (
'verysmall': (max-width: 390px),
'smaller': (max-width: 520px),
'small': (max-width: 830px),
'medium': (max-width: 1200px),
'large': (max-width: 1400px),
);

@mixin respond($breakpoint){
   @media (map.get($breakpoints,$breakpoint)){
         @content;
   }
}

    see it does the same thing your code did, and the inclusion is alike.

    @include respond('large'){
width:90%;
}
    Marked as helpful
    1
  • ashleyftw•120
    @ashleyftwSubmitted over 1 year ago

    Not 100% sure about the background pattern... What's the best way to scale svg? Also any tips on how to organize code, especially with SASS, would be appreciated!

    responsive page using SASS

    1
    Shalom2935•420
    @Shalom2935Posted over 1 year ago

    If you want a good organization i'd like to suggest the 7-1 pattern which will make you codebase very clean and easy maintainable. check it here

    0
  • Shalom2935•420
    @Shalom2935Posted over 1 year ago

    Good job I really like the way your code is structured.

    • Wrap all the code inside the body with a main element. By using landmarks you improve the code accessibility.
    • You don't need to give the body a max-width of 1440px, on the contrary this can become an issue for the responsive. Instead, a width: 100vw will ensure that the body will take all the width avaible on the viewport.
    • To make an image responsive, don't set its height!!! give it 100% width and give its container the width you'd want your image to have and a height set to auto.

    Hope my comments are helpful. HAPPY CODING

    0
  • Shalom2935•420
    @Shalom2935Posted over 1 year ago
    /* remove spin buttons */

/* Chrome, Edge, Opera*/

input[type=number]::-webkit-inner-spin-button {
    
    display: none;

}

/* Firefox,  Safari */

input[type=number] {
   
    -moz-appearance: textfield;
    -webkit-appearance: textfield;
  
}

    This will help you remove the spin buttons while hovering or focusing on the input. This solution is the prettiest one I have seen that far and I'm literally fan of the animations. I just figured out a situation I have yet to take into account and it seems you also missed it: What will happen if not the year but globally the input date is higher than today? maybe 5 april 2023. That must be fixed.

    Marked as helpful
    1
  • Shalom2935•420
    @Shalom2935Posted over 1 year ago

    it wronged my age. It considers many conditions but you forgot the 0 value. And also your site is far from the design.

    • You use the button element as a container of the arrow but that won't work. the line is a div element. It must be sibling with the arrow, then push the arrow by increasing the line width.
    0
  • Shalom2935•420
    @Shalom2935Posted over 1 year ago

    It wronged my age. And there are many rooms to improve, nothing seems to work properly. the inputs, the errors conditions, the outputs. So keep working and fix it.

    0
  • Hoshiko•180
    @oliwiakrammSubmitted over 1 year ago

    👩‍💻My solution to this challenge👩‍💻

    First time using API in a project.

    Additional things:

    • button is disabled for 2 seconds after fetching an advice
    • advice text animation

    Thank you for any feedback 😁

    Advice generator app - JavaScript, Sass, BEM

    2
    Shalom2935•420
    @Shalom2935Posted over 1 year ago

    beautiful but there is a problem with the animation.

    0
  • Nathasha•150
    @NathashaR1997Submitted over 1 year ago

    Hey Everyone, I have completed the Testimonials Grid Section challenge.

    Check out my following article, where I have briefly explained the steps I have followed to complete this challenge.

    https://medium.com/ux-planet/challenge-010-testimonials-grid-section-9067105b9ff

    Thank you for checking this out, and feel free to leave your feedback and thoughts!! Any feedback and tips are welcome.

    Many Thanks! Nathasha 😊

    Testimonials Grid Section Using Pure CSS

    1
    Shalom2935•420
    @Shalom2935Posted over 1 year ago

    Hey @Nathasha, I really like the way you explain every step of your code. Some ideas to improve your code:

    • Wrap the code inside the body in a main element instead of a div; that will make your code more semantic therefore fix the accessibility issue.
    • The .parent-wrapper element is too wide, give it a width of 80% instead of max-width:1440px.
    • I think doing it with flexbox is too much work, learn the css grid and you will see how simple it could have been.
    • There is quote in the background of .daniell-tetional-wrapper that you missed. Two ideas to help you choose what suit you the most. 1- give the .daniell-tetional-wrapper a relative position, give the quote you want in the background an absolute position; use top and right properties to adjust his position, then use the z-index so that the quote will be beneath the text. 2- place the quote as the background-image of .daniell-tetional-wrapper, then adjust his position with background-position: top; and background-position-x: "value_that_will_work".
    • Your responsive is designed only for large and small screen's size, it would be very nice if you added a medium size screen.

    I hope my comments will help you somehow, happy coding journey.

    1
  • Shalom2935•420
    @Shalom2935Posted over 1 year ago

    Hello @Tosinjoseph these some advices to improve your code:

    • Change h2 to h1 and give your img an alt to fix the accebility issues.
    • Don't give a fix height and width to .all cause it will prevent your site from being responsive, if you want it to take all the screen size you can use the css code
    body{
      height: 100vh;
      width:100vw;
}
 .all{
     width:100%;
     height:100%;
}
    • It must have been really tiring to give all these margin and paging in order to dispose your element on the page; that is not a good habit you will struggle a lot doing it every time and it won't even work. Instead, it will be better for you to learn css flexbox, you will have a lot of fun using it and it will save your time.
    • You might have noticed this but the color you used on the .container background is wrong.

    Hope I have help you somehow, and you will have a happy coding journey

    0