Skip to content
OverviewSolutions
0
Comments
8
Sönke Martens
@Ribosom

All comments

  • Sönke Martens•120
    @RibosomPosted almost 3 years ago

    Hi there, nice solution! And responsiveness is really nice!

    Some advices:

    • I think the boxes should have a white background
    • The paragraph below the main heading, is too long. It is hard to read if lines are too long. You can see in the design, that it should be shorter. You can do something like max-width: 55ch;
    • I think it would help, if you format your code html/css probably, so it is easier to read for yourself and others. (Using autoformatting of an IDE like vscode can help)
    Marked as helpful
    1
  • Sönke Martens•120
    @RibosomPosted almost 3 years ago

    Hi, nice solution!

    Since you asked for suggestions, here are some ideas:

    • Using only h1 is not correct semantic html. A page should have only one h1.
    • Working with max-width, and max-height (instead of width and height) would allow the boxes to shrink on very narrow screens
    • I think the heading on big screens is smaller than in the design.
    • And the background color of the boxes is white in the design, isn't it?

    Everyhting else is very nice. :-)

    And some other approaches (which are not strictly better, but maybe give new ideas, how to approach things differently):

    • The color on top of the boxes, can also be solved with ::before pseudo element.

    For example:

    .box::before {
    display: block;
    position: absolute;
    content: "";
    background-color: var(--primary-blue);
    left: 0;
    right: 0;
    top: 0;
    height: 0.3125rem;
}

.box--supervisor::before {
    background-color: var(--primary-cyan);
}
    • The icon can also be placed with ::after pseudo element.
    .box::after {
    float: right;
    margin-top: 2rem;
}

.box--supervisor::after {
    content: url("images/icon-supervisor.svg");
}
    • Instead of flexbox, a grid with 4 rows and 3 columns can also do the trick
    0
  • Jaycee Altamia•330
    @simplyJCSubmitted almost 3 years ago

    This is my first challenge in JavaScript and there is sill a lot of room for improvement. This is not the best solution but this is how I approach and solve the problem in my own way base of the current knowledge that I have. I am open to any suggestion or feedback. Thanks. (I changed the JS code to a much simpler one.)

    Intro component with sign up form using BEM, Sass, JS and mobile first

    2
    Sönke Martens•120
    @RibosomPosted almost 3 years ago

    I like the solution. However, I think it would be better, if the validation errors are updated when the user fixes the problems and not when he submits again.

    Adding "input" event-listener could help you refresh the classes when the user changes the content.

    You also could check my response to another submission, where I outlined my approach, maybe there are some ideas you like.

    Marked as helpful
    1
  • Sönke Martens•120
    @RibosomPosted almost 3 years ago

    I really like your solution, especially the animations (great idea). From a design perspective, I like it pretty much, even if it is not exactly like the template.

    Regarding the validation, I would do it differently (but you have to decide yourself, if my solution can help you) Maybe some ideas work for you as well (The ideas are copied from my solution, which I didn't submit^^) I really think you could simplify your html/js with a little different approach. And then not only one error message, would be shown on submit:

    Use real submit input (advantage, click listener would also be activated on submit with keyboard)

    <input class = "..." type = "submit" value = "Claim your free trial">

    I added a event listener, which adds a "submitted" class to the form (helps to react on form submit in css)

        document.querySelectorAll("input[type='submit']").forEach(input => {
        input.addEventListener("click", () => {
            input.form.classList.add("submitted");
        });
    });

    Similar I added classes, when the inputs are changed or when validation is invalid. You can even put different invalid classes depending on the validation error by using input.validity in JS (I used the <input type="email" ...> default validation, here your solution might be better):

        document.querySelectorAll("input").forEach(input => {
        input.addEventListener("input", () => {
            refreshInputInvalidClasses(input);
        });
        input.addEventListener("invalid", () => {
            refreshInputInvalidClasses(input);
        });
    });

    function refreshInputInvalidClasses(input) {
        if (input.validity.valueMissing) {
            input.parentElement.classList.add("invalid--required");
        } else {
            input.parentElement.classList.remove("invalid--required");
        }

        if (input.validity.typeMismatch) {
            input.parentElement.classList.add("invalid--type-mismatch");
        } else {
            input.parentElement.classList.remove("invalid--type-mismatch");
        }
    }

    And then I added the error messages as custom html attributes, which can also be read in css/scss. For example for the email:

                    <div class = "input-container"
                     data-error-required  = "Email Address cannot be empty"
                     data-error-type-mismatch  = "Looks like this is not an email">
                    <input class = "input-field"
                           name = "email"
                           type = "email"
                           aria-label = "Email Address"
                           placeholder = "Email Address"
                           required>
                </div>

    When using pseudo elements, you even don't need the validation messages as separate divs/spans, but can read them with attr in css/scss:

    form.submitted .input-container.invalid--required::after {
    content: attr(data-error-required);
}

form.submitted .input-container.invalid--type-mismatch::after {
    content: attr(data-error-type-mismatch);
}

    The error icon can be set as background-image, which may be simpler:

    form.submitted .input-container .input-field:invalid {
    ...
    background-image: url("images/icon-error.svg");
    background-repeat: no-repeat;
    background-position-x: calc(100% - 24px);
    background-position-y: calc(50%);
}
    Marked as helpful
    0
  • Suprefuner•470
    @SuprefunerSubmitted almost 3 years ago

    Start learning HTML and CSS a month ago and fall in love with it. First time submitting challenge, please let me know if there is any better way to code.

    Things I will change next time,

    • separate css file and html file, bad habbit.
    • start with mobile version first

    Frontend mentor challenge - order summary card

    2
    Sönke Martens•120
    @RibosomPosted almost 3 years ago

    Hey there, great solution!

    Here some smaller feedback:

    • The icon is missing next to the "Annual plan"
    • The background-color of the hover effect on the button and the link is slightly off. I think it is not supposed to be grey but a lighter (or more transparent?) variant of the "brighter blue" color.
    • The background is not repeating on bigger screens. I would propose to make it repeating in x axis. You can use background-repeat: repeat-x; for that.
    • You could consider, using semantic html like main or header to improve accessibility.
    Marked as helpful
    0
  • Sönke Martens•120
    @RibosomPosted about 3 years ago

    Hi @Denismapll, nice solution!

    One idea for your css. I would do one container class for the stuff the three container share and then three different classes only for the background-color for example bg-bright-orange, bg-dark-cyan and bg-very-dark-cyan.

    In the html you could then do for example <div class="container bg-bright-orange">.

    This has the advantage, that if you have to change the css of the container, you don't have to change it three times. Instead of making three button classes button1, button2, button3, you could use the same principle and add the different colors via descendant combinator:

    .bg-bright-orange .button {
   color: var(--cc-bright-orange);
}
    Marked as helpful
    1
  • Sönke Martens•120
    @RibosomPosted about 3 years ago

    Hi @thalesmaiaa, nice solution.

    Two minor problems, which could be improved: You could use box-shadow to add the missing shadows. Also the background-color change seems to be missing on the hover effect (on the "Change"-link and the upper button)

    Some css advice, you can use variables to help reuse values like colors. Adding variables to the root element can help a lot and makes it easier to change colors later if needed.

    example:

    :root {
    --color-primary1: hsl(225, 100%, 94%);
}

    and use them on other parts in css with var. For example:

    background-color: var(--color-primary1);

    0
  • saira•130
    @saira512devSubmitted about 3 years ago

    1.Since i dont have a 'designer's eye', i would be really grateful if anybody points out the details i have missed out and the mistakes 2.What should be the average time taken to complete a challenge?

    Responsive page using css flexbox

    1
    Sönke Martens•120
    @RibosomPosted about 3 years ago

    Hey there, I am no designer myself, but regarding your first question:

    • You forget the shadows of the panel and the "Proceed to Payment" button.
    • The underline under the "change" link is slightly higher. (You could use text-underline-offset for that)
    • The font size of the buttons is too large.
    • The color of the hover effect is off. I think (not sure myself), but for me in the design it looks more like a transparent version of the color primary 2.

    Sadly the second question I can't answer, since I am also new here, but I would be also interested in that. However, I am a (backend-)developer and I think you shouldn't stress yourself on coding-challanges in the beginning, try to concentrate on learning as much as possible. You will get faster with time and more experience.

    Marked as helpful
    0