Skip to content
OverviewSolutions
16
Comments
31
WandoCode
@Wandole

All comments

  • WandoCode•840
    @Wandole

    Hey,

    Very nice!

    For your problem with the button "Register" on hover: it's because the hover effect remove the border on the button. The border is 2px wide and for that reason the 'login' button shift to the right by 2px...

    To fix that, you could keep the border on the hover effect (for the 'Register' button), but change the color to 'transparent' for example!

    .btn:hover{
    border: 2px solid transparent; // Or even 'border-color: transparent' is enought I think
}

    I hope it helps!

    Marked as helpful
  • WandoCode•840
    @Wandole

    Hey,

    when I am exactly with a window of 1440px, the card is centered. However, as soon as I have a bigger window, it's not centered anymore. Worst: when the window size is under 1440px, your card zoom out...

    To fix that, you should avoid to use fixed width/height. Use min/max-height and min/max-width! It's an important rule in CSS. There are a lot of exceptions of course, but keep that in mind.

    For example, to center your card ( your ".main"), remove the "width" on the "body" tag. As you have set "margin: auto" to the ".main", it will center the ".main". You don't need media queries anymore with that!

    The card itself was quite good!

    Have a good day!

    Marked as helpful
  • WandoCode•840
    @Wandole

    Hey,

    the design of your solution is very good IMO!

    The ways you could slightly improve it:

    • when I click the main input to write a bill amount or the nbr of people, there is a shift of about 1mm of all your design. I didn't look into you CSS, but I suspect that it comes from a border that you add around the input. Try to use an outline instead of a border.
    • for the tip amount when I hover the buttons (5%, 10%, etc.), the mouse cursor don't show me that I click the element (add the CSS propriety 'cursor: pointer')

    For the logic of your app, you should improve it:

    • The amount is calculated when I click on 'reset', I would like to see it before
    • The values displayed are not good...
    • I can't reset the form if there is a error in the form (if I let an input empty for example)

    But overall, it's very good. Your design is close to the one you received!

  • WandoCode•840
    @Wandole

    Hey!

    As a user, I would like to actually see the choice I made when I click on a number. You should give a different style to the button when it's clicked. Talking about the button, in your code there is a list of choices (from 1 to 5): from an HTML point of view it's look a lot like radio inputs! You should try to use that instead of multiple buttons. It will also help you for the style of the selected value! You will need to make some search to understand how to style radio inputs but it's worth it because you will use it A LOT in your next projects.

    The style in general looks very good! Maybe try to center better text in buttons? You should try to use Flexbox for that for example.

    And for all your project, avoid using a fixed width (% is okay though but don't fit in all the situations) because you will have overflow issues with that on small screen sizes! It's not an issue in that project though :) Instead, use max-width as soon as you can! Same for height => use min-height.

    Sometime fixed size is mandatory, but when it's not, don't use them, it will save you a lot of time!

    I hope it help!

  • WandoCode•840
    @Wandole

    I'm not sure to understand what you meant about the window size.

    If I was you, I would set a "min-height" propriety to your .card (you did well for "max-width"). You used percentages for the height so the height of your card depends on the height of the user's screen (because the .card parent is the <main> and you gave him a height of 100vh, which was a good idea). In definitive, you can't be sure about what the user will see on his screen!

  • WandoCode•840
    @Wandole

    For your JSON file, in plain JS, you can fetch it .

    
const rep = await fetch('./datas.json');
const datas = JSON.parse(rep) 
// Or 'const datas = await rep.json()'
// I don't know which one is better. Probably the second one if the JSON have a lot of datas 
// because it would take some time to parse it.
    Marked as helpful
  • WandoCode•840
    @Wandole

    Hey,

    For your question: I think the "thank you" screen is fine like that! In a real project that card could be a part of a page or be a modale: it's normal to just change his content!

    But you should set the 'How did we do' screen in the same card than the 'thank you' screen.

    Something like:

    main
    cardContainer
        howDidWeDoScreen
        thankYouScreen

    And you toggle the display of 'howDidWeDoScreen' and 'thankYouScreen' accordingly of what you want to show.

    This way you can style the 'cardContainer' for centering, size, responsive, etc. And the 'howDidWeDoScreen' and 'thankYouScreen' are styled separetly for their inner content (and the 'display' property).

    In my opinion, it's easier to maintain and the html structure have more sense like that! To add even more semantic to your HTML, you could use a 'section' for the card instead of a 'div' maybe?

    I hope it helps!

    Edit: you made a good job I think! Just be carefull of 2 others things if you want to improve your skills:

    • Don't use the * (except for very general rules): it can slow rendering and will make your code less readable. For example: color and font-family can be set in the 'body' => The children elements will inherit the property!
    • When I click the submit button whitout selecting any rating, I can access the 'thank you' screen.

    Like I said (and even if I made a lot of commentaries ...), you've made a really good job. I only wrote all things I think you could do to improve your project, or for future ones!

  • WandoCode•840
    @Wandole

    Hey,

    For me everything seems fine. It's even well responsive!

    The only thing I could say is about align-items: center; justify-content: center;. They are useless for me.

    Keep it up :-)

    Marked as helpful
  • WandoCode•840
    @Wandole

    Hey!

    This solution is good.

    The only advice I could give you for next projects, mainly for bigger ones: when you use BEM, try to avoid 'nesting' on your classes name.

    For example:

    • 'card__content__title' should be 'card__title' in my opinon.
    • same for 'card__content__description' => 'card__description'

    If you really need nesting, it means that you need to create a new component!

    It's just a detail that will make bigger project more readable.

    I hope it helps!

  • WandoCode•840
    @Wandole

    Hey,

    You've made a good job I think!

    • the way to show/hide the different sections is good
    • the JS is good too

    I have some advices if you want to improve your solution (it's mainly about semantic), in my opinion, I let you decide what is relevant:

    • you should use a form tag to wrap the rating values (your buttons)
    • you should use radio input instead of buttons to get the rating value (it's a unique choice among multiples values). If you never styled radio inputs, you can read that to have an idea of how to do it. It's very usefull and you will use it a lot in future projects.

    Also:

    • the user should not be able to display the 'thanks' screen if he have not selected a value before.

    In term of best practices:

    • the CSS variables have to be declared in the scope where they will be used (usually all the HTML)==> you should use the :root selector. You can read the begining of that article if you want more informations.
    • your CSS variables are not very reusable. If I give you a 'light themed' design of the same project, you will have to change their name. To avoid that, you can use 'abstract' names, for example:
    :root{
   --clr-primary-400: hsl(25, 97%, 53%); // Your orange variable

   --clr-neutral-100: hsl(0, 0%, 100%); // Your white variable
   --clr-neutral-200: hsl(217, 12%, 63%); // Your light-gray variable
   --clr-neutral-300: hsl(217, 12%, 63%); // Your light-gray variable
 ... etc.
}

    It's only a detail, but for bigger project with 2 themes (light/dark) for example, it will be usefull maybe!

    I hope it will help!

    Marked as helpful
  • WandoCode•840
    @Wandole

    Thanks for the answer!

    If you only used what I wrote, it's normal: the .left-card is the container of the card on the left (the 1st row) and it places only that card correctly.

    You have to create something quite similar for the card on the right! It's almost the same except for the grid-column property.

    https://codepen.io/timanfaya2/pen/KKZMebJ

  • WandoCode•840
    @Wandole

    Hola!

    Para empezar, espero que vayas a entender todo lo que voy a escribir porque el Español no es mi idioma maternal y no estoy acostumbrado de explicar cosas de informática en español :o

    • Para hacer el hover, puedes poner una div justo alrededor del tag <img.../>. Tiene que tener exactamente el mismo tamaño que el imagen.

    Esa div va a servir para crear un pseudo-elemento before:

    div {
   position: relative; // Eso es util porque voy a utilizar una posicion 'absolute' en el pseudo- 
                                   // elemento ==> Asi la posicion del pseudo-elemento sera calculada según  
                                   //la 'div'
}

div:hover:before {  
   content: '';         // Si no pones eso, el 'before' no existe !!!
   position: absolute;
   top: 0;
   bottom: 0;
   left: 0;
   right: 0;              // Esos '0' sirven para dar la posición del 'before' en la 'div' ==> por ejemplo, el 
                               // tope (top) del  'before' esta a 0px del tope de la div 
                               // Con las 4 posiciones a '0', el 'before' va a tener la misma posicion y el mismo 
                               // tamaño que la 'div
   background-color: blue;
   opacity: 0.5;
}

    Si quieres añadir el imagen del ojo, tienes que ponerla en el content: url(...).

    • Una otra solución (sin pseudo-elementos) es de crear la div y poner otra div (vacia) dentro:
    <div>
   <img.../>
   <div ...> <div/>
</div>

    Puedes utilizar esta segunda div como utilicé el before. Aqui, puedes colocar la imagen del ojo en la segunda div ...

    En los 2 casos, tendras que poner el ojo en el centro pero te dejo buscar por eso!

    Espero que eso vaya a ayudarte (y que me has entendido haha)!

    Marked as helpful
  • WandoCode•840
    @Wandole

    Hey,

    For the form validation, you could validate each field separately in a given function.

    To make the validation:

    • the name field => just check the length of the value (string)
    • the email field => you can use a RegEx and the method <string>.match()
    • the phone field => use an input:text and RegEx/match too!

    When a field doesn't pass the validation, you can add the name of this field in an array/object and when every fields has been checked, you return the array/object to your main function.

    That way, if your validation function return an empty array, you know the validation has passed, otherwise, you have the name of the field(s) that are not valid and you can display that on screen using a state!

    There are probably other ways to do that, but that's how I do it!

    I hope it helps!

    Marked as helpful
  • WandoCode•840
    @Wandole

    You have a lot of (good) questions. I'll try to answer to some of them.

    • The button position

    To use position:absolute, you have to set position: relative to the button's container (in your HTML, it's .container). After that, you can use 'tops, bottom, left, right' to position the button inside of that that container. Currently you have not set any position: relative so the button is position relatvely to the body tag (so the screen).

    Sadely, your .container take +/- all the screen, so it's going to be difficult to position the button inside of it, even if yout set position: relative to it.

    To make that easier, you could put the button inside of the .generator-box and set position: relative to it. This way, the button is positioned inside of the .generator-box !

    • SVG size

    You should use the img tag to load your divider.svg. This way, you will be able to resize it I think.

    • Layout shift

    I don't see the layout change with the text length, maybe you already resolved that!

    • Media queries

    It should work I think. Maybe the mobile you used to test it have a larger screen than the value you used for your media query?

    I hope it will help!

    Marked as helpful
  • WandoCode•840
    @Wandole

    Hey!

    Overall it's quite good!

    As Peter said, there is a bug in Step 3. And I will add an other problem with your form validation (Step 1):

    • the 'name' field is good!
    • the 'email' field is valid even if I write something without '@' into it.
    • the 'phone' field: the placeholder is '+1 234 567 890' but I can't write that into the field since it have the type 'number'

    For those 2 problems, you could use an input:text and make the validation with RegEx for example!

    I hope it will be usefull !

    Have a good day :-)

    Marked as helpful
  • WandoCode•840
    @Wandole

    Hey,

    As you asked for the card position, I looked into your CSS to know how you did it. The method you used is good, but it's really not the best (I mean, there is easier!).

    • for horizontal centering => if your card have a width/max-width, you can simply add an auto margin on right and left. It's easy and short:
    .card {
   max-width: 15rem;
   margin-inline: auto; // It's a shorthand for margin-left and margin-right
}
    • sadely, I think that you can't use the same method for vertical alignment. For that, you can use felxbox (and it works for horizontal alignment too):
    .card-container{
   display: flex;
   justify-content: center; // horizontal alignment
   align-items: center; // vertical alignment
}

    For both methods, the card will be centered into its container. So be sure that this container take all the screen (if you take the body as the container, it's already good but if you want to use the main tag, you will have to adapt it to take all the screen).

    The 'position: absolute' property can be usefull in HTML/CSS but it's better to use it only when you don't have other choices because it can lead to problem with responsivness.

    I looked your HTML too to check the semantic and you've made a good job, I think. There are maybe some details to change but I'm not even sure.

    I hope i will be usefull :-)

    Have a good day!

  • WandoCode•840
    @Wandole

    Hey,

    Thank for your advice about the filter. It's probably better to change the 'None' to avoid confusion.

    For the 'Dark mode' text, it's in the design: the text stay the same for both theme... When I coded that part, I found it weird too but I thought about it and finally I decided to just follow the design !

  • WandoCode•840
    @Wandole

    Hey,

    To add the shaddow behind the picture, you can use the ':before' on your div.image (not on the img itself since the tag can't have it).

    You put 'inset: 0' (shorthand for 'bottom', 'top', 'right', 'left') as property in the 'before' element and set a background color with opacity <1.0.

    If you never used ':before', got check how it work before! (You will need to set a content and the 'position' property if you want it to works).

    Otherwise, you can, as you said, create another div to make the shaddow. You place it inside of your div.image container and, as for ':before', you use a 'position:absolute' on it with 'inset:0' and a background with opacity.

    I hope it will help you!

    Marked as helpful