Skip to content
OverviewSolutions
24
Comments
29

Tryt4n

@Tryt4n940 points

I’m a mysterious individual who has yet to fill out my bio. One thing’s for certain: I love writing front-end code!

Latest solutions

View more solutions

Latest comments

  • Tryt4n•940
    @Tryt4n

    Because you declaring width and height as a percentage, when the content is bigger then it's overlapping or when the screen size is narrow and tall it's just look not nice. You should avoid declaring width and height based on percentages values to avoid those kinds of situations. They are very problematic in terms of responsiveness. Just declare a constant value or use min-width, max-width, or min max values.

    Your email validation checking only for gmail, hotmail and yahoo mails. Instead of doing some things with regex on your own there are easier way to do this. Just google out regex for email or use library like email-validator.

    You can also consider using a <dialog> element for your confirmation element. It's way better and way more accessible.

    Tip: In your case if you want to center element in the page the easiest way is to use dispaly: grid and place-content: center or place-items: center on your container element so in this case on <main> element. It's easier than declaring top and left with some values and then doing the math.

  • Tryt4n•940
    @Tryt4n

    Your path for ::before pseudo element in blockquote is wrong. Personally I think it would be better use ::before or ::after on your person img instead of adding another img element which has only a decorative function.

    Also would be better if you would add your buttons in img section. Would be easier to place them. Currently they always have different placement. Instead of that it they would be in img container you could position them absolutely to that container then add bottom: 0; left: 0; and translate: 100% 50%; and it would be exactly the same as in the project.

    Also because you use top and left on your buttons container with such a massive number (40rem and 60rem) it's completely mess with your media query. Below 1100px your buttons overlapping your body.

    Your media query for mobile should start below around 1024px in my opinion. Even when I disabled 60rem for left for your buttons the img element is getting started to be very small.

    And some typo:

    • your both buttons have the same aria-label
    Marked as helpful
  • Tryt4n•940
    @Tryt4n

    Between width of 480px to around 700px on your social link icons narrow because you use width: 30% on .footer__social. Also for <form> element you could add some padding-inline because below 580px elements inside touch the edges and personally I would add text-wrap: nowrap; on your button because between 480-570px text is wrapping. For image you could change it so that it is always the same width as the form.

    For accessibility:

    • text "soon" in your h1 could not be visible for assestive technologies. Instead of that you could wrap it in strong or b or just leave it as it is and add <span class="visually-hiden>soon</span>
    • label is empty. You could add span with visually-hiden class and some text like "email"
    • add aria-errormessage="some_id" to your input
    • to you warning text add this id and aria-live="assertive"
    • © symbol is the same as &copy ; but without space between letter "y" and sign ";"
    • you could add some description for your footer navigation like aria-label="social links navigation" because in real website it would be probably just a component and there would be another nav elements

    Personally I would change span for p with your text "Subscribe and get notified. p is for paragraph and span is just like div but it has display: inline;

    Marked as helpful
  • Tryt4n•940
    @Tryt4n

    You can add <time datetime=""> element instead of <span>. For button you can add aria-haspopup="true".

    Also your media queries in my opinion are set wrong. About 860px width and below it's overflowing. If popup is open overflow starts around 940px.

    Marked as helpful
  • Tryt4n•940
    @Tryt4n

    just use:

    .mainscore {
   display: grid;
   place-items: center;
}

    instead of what you're doing. I think it's the easiest way to center, just two lines of code.

    Marked as helpful
  • Tryt4n•940
    @Tryt4n

    For accessibility you can add role="region for every accordion__box because all elements inside are related to each other. Then add aria-expanded="false" for every accordion__head and handle change state with JS when it's expanded/collapsed. Also for every <p class="accordion__description"> add id and then for corresponding <div class="accordion__head"> add attribute aria-controls="your_id".

    In your case user cannot use keyboard to navigate so there are two options:

    • change div class="accordion-head" for button
    • add role=button and tabindex="0" to <div class="accordion__head">
    Marked as helpful
View more comments