Skip to content
OverviewSolutions
24
Comments
29
Tryt4n
@Tryt4n

All 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
  • Tryt4n•940
    @Tryt4n

    You've got some problem with fetching due to cors. Probably due to by IPify API. I had the same but used a different API

    And also you are using React. None of your images like marker are working. All your resources like images should be in public folder and then you acces them by img src="/example-picture"

    Marked as helpful
  • Tryt4n•940
    @Tryt4n

    You've got some problem with fetching due to cors. Probably due to by IPify API. I had the same but used a different API

    Marked as helpful
  • Tryt4n•940
    @Tryt4n

    You should wrap everything in main. If you decide that the one content is more important than the other you can wrap this/those element/s in aside.

    aside element doesn't have to be literally aside. It's just less important content on the page.

    Also you skipped heading level in newsletter. There should be h3 instead of h4. If you want write semantic HTML with accesibility you should use some evaluation tool. I personally use wave. If you want check it out.

    It would be good to use a for thing like phone number or email. You use a for email but it should look like this: a href="mailto:example@huddle.com". Same with phone number: a href=tel:123456789. Change this and then click on those elements and you'll see why it should be like that.

    What do you mean that you had problem with fonts?

  • Tryt4n•940
    @Tryt4n

    In overall it's well written. BEM class architecture is good but folder structure could be better.

    src folder isn't asset

    In assets folder should be images, fonts, etc.

    For colors you could make new colors file

    In base folder you could split the base file into several different files. For example base, font-face, typography, helpers

    If you would have helpers file you could put there visually-hidden class and use it in your h1 instead of card__title--hidden because it's not modifier. You are just visually hiding this element from the user not modifying it.

    Marked as helpful
  • Tryt4n•940
    @Tryt4n

    It's because you gave min-height: 100vh; for your body. Change it for display of flex, flex direction of column and justify-content of center. Then delete margin-top from your footer and it would be done.

    Nice trick for debugging CSS:

    * { background: hsla(100, 50%,50%, .2); }

    You can use something like this. It gives opaque background to everything on your page. So the backgrounds of the elements will overlap each other. Thanks to that you will be able to see where the problem is placed. Before you edit your solution just check it out. Also if you have some pseudo elements ::before or ::after also give them the background. *::before and *::after

  • Tryt4n•940
    @Tryt4n

    Firstly for your whole element you gave max-width: 25vw that's mean if you have mobile with width of for example 400px the element width would be 100px. You could do something like this:

    min-width: 25vw;

    max-width: 500px;

    That's mean width would be 25vw but no more than 500px. Of course you can change those values.

    Another thing add to your img line margin-inline: auto; because it's not centered. That's mean add the same amount of margin in the left and the right.

    Last thing add text-align: center; to your whole element so the text content would be centered.

    Marked as helpful
  • Tryt4n•940
    @Tryt4n

    It's becasue you write it to be color changed: .nav-toggle-label:hover { background-color: rgba(0, 0, 0, 0.1); }

    Also there is no such a thing like label type="button" You should use button instead of label. label element is for inputs to describes what that input is doing. Something like header for input, and that's why label has for="" attribute. This is where you type id of input that should be labelled by that label.

  • Tryt4n•940
    @Tryt4n

    Firstly in your header navigation aria-label is not needed, because element nav explain itself what it is. Nice though with aria-hidden with img or label without text. Another thing with your footer. a elements with email phone etc. could have something like this: <a href="tel:+48123456789">+48 123 456 789</a>

    <a href="mailto:example@example.com">example@example.com</a>

    And with that if you click on that element it will automatically open application for phone calling or email etc. In overall it's good.

    Marked as helpful
  • Tryt4n•940
    @Tryt4n

    Your button on hover state overlaps on input email. Also in footer lorem text have additional margin top and bottom and it's look weird. You could also add padding left and right to your footer section because in width < 1300px content is on the edge. The same with your testimonials section. On hover state in mobile it's on the edge.

  • Tryt4n•940
    @Tryt4n

    In your section-info class would be better to use grid instead of divided it into two div and use flexbox on them. You just could do something like this:

    display: grid

    grid-template-columns: 1fr 1fr;

    grid-template-rows: 1f 1ft;

    And then it would be easier in mobile view:

    grid-template-columns: 1fr;

    grid-template-rows: repeat(4, 1fr);

    Also you could add some transition while :hover elements. Now it's just so instant especially in your button. Give transition: 300ms ease-in-out and would bo smooth.

    About accessibility you done good job with creating a label although it has no content, but in that case you should give it aria-label: "Enter email"

  • Tryt4n•940
    @Tryt4n

    Visually it's good but you should add alt tags. It's for alternative technologies and if there is # it would be ignored. Just type fylo logo, folder icon etc. And also you can put text 815GB in em or strong element instead of span. em is emphasize and strong mean it's something important on the page. In this case I think em would be better.

  • Tryt4n•940
    @Tryt4n

    You can also check if page's user have light or dark device color scheme.

    if (!document.body.classList.contains("")) {

    let dark = window.matchMedia("(prefers-color-scheme: dark)").matches;

    if (dark == true) {

    body.classList.remove("bg-white")

    body.classList.add("bg-dark")

    } else {

    body.classList.remove("bg-dark")

    body.classList.add("bg-white")

    }

    Marked as helpful