Skip to content
OverviewSolutions
13
Comments
16
Bilal Türkmen
@bilalturkmen

All comments

  • Bilal Türkmen•340
    @bilalturkmenPosted almost 2 years ago

    Hi, nice work.

    I have a simple suggestion for the footer section to be at the bottom.

    you should delete this line

    and change this line

    like this justify-content: space-evenly;

    i think it will look better 🤙

    Marked as helpful
    1
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    Hi Alper nice work.

    positioning in css sometimes can be confusing. I also experience this situation often.

    We need to add a relative container before the absolute position. actually you already added.

    so you can uncomment again this div

    <div class="hero__img-area">
<picture>
              <source
                media="(min-width: 62.5em)"
                srcset="./assets/img/desktop/image-hero-desktop.png"
              />
              <source
                media="(min-width: 37.5em)"
                srcset="./assets/img/tablet/image-hero-tablet.png"
              />
              <source
                media="(min-width: 31.25em)"
                srcset="./assets/img/mobile/image-hero-mobile.png"
              />
              <img
                src="./assets/img/mobile/image-hero-mobile.png"
                alt="The hero image"
                class="hero__img"
              />
            </picture>
</div>

    and then define the following css properties

    .hero__img-area{
  position:relative
}

.hero__img{
    position: absolute;
    top: -18.25rem;
    left: 0.1875rem;
}

    delete this

    update your .wrapper class min-width: 100% and .container class width:70em

    then set the "top" and "left" positions in the .hero__img class for your media queries.

    Secondly... I think this accessibility message pops up because you used nav tag twice. if you use aria-label for these tags, i think it will fix the problem.

    example:

    <nav aria-label="header menu"> ... </nav>
<nav aria-label="sub menu"> ... </nav>
    Marked as helpful
    1
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    nice and meticulous work.

    Custom scrollbar is a nice detail. I liked framer motion, want to try too 🙂

    1
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    Hi,

    • for the first question, it seems ok.

    but if you want use less css code, you can check this example

    • I don't know much about styled css, but i find it more correct to use external css, for file size and readability by others. but it seems be comfortable to use for small applications

    • maybe you should try css module. there is a comparison

    Marked as helpful
    0
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    Hi Alper, nice work.

    It might be better, adding this background color to the body selector, instead section. White space appears around the edges of the page in full hd resolution.

    Happy Coding 🙂

    Marked as helpful
    0
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    it makes me want to learn typescript 🥳

    this style blew my mind 👍

    1
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    you are doing this job 👏

    1
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    Hi, nice work. i think it can be improved a bit 😊

    no need to use js to center an element.

    ✅css grid or flexbox is enough for this. to center an element over the viewport area you should add min-height property to body selector like this

    body{
min-height: 100vh;
    display: grid;
    font-family: "Hanken Grotesk", sans-serif;
    text-align: center;
    place-content: center;
}

    and then you can delete the index.js. you dont need that anymore. you can also delete this div

    • your media query breakpoint stays small for the width of the card design. should be expanded a little more like this
    @media screen and (min-width: 768px) {
}

    ✅ can be replaced as main because document should have one main landmark.

    • same this div

    ✅ can be replaced as h1 because page should contain a level-one heading

    i hope these helpful 🙂

    Marked as helpful
    0
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    Hello, nice and clean work.

    i think maybe it can be improved a bit 😊

    • Collecting css resources in a single file can improve page load performance. Likewise, instead of calling the externally font as @import, adding it as a url inside the <head> tag can prevent slowdown.

    • Product images can be use with <Picture> element instead of css background, it allows you to define "alt" tag. This provides positive benefits in terms of SEO and screen readers accessibility.

    <picture>
        <source
          media="(min-width: 42.5em )"
          srcset="./images/image-product-desktop.jpg"
        />
        <img
          src="./images/image-product-mobile.jpg"
          alt="in 100 ml glass bottle - channel - eau de parfum"
        />
      </picture>

    • You can use for primary font font-family property in body selector. You don't need to add font-family to other classes again and again. After adding font-family to body, you can delete the others.

    • and this happened to me too :) for screen readers an sr-only text can be added before to the old price.

    Of course, there are here more experienced people than me. Maybe they can say different things.

    i hope these helpful 🙂

    Marked as helpful
    1
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    Wow impressive 🙌

    1
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    hi Aj, nice work.

    you define background theme on tailwind config, but it seems little bit false. maybe you can try like below

    backgroundImage: {
        "desktop-pattern": "url('/src/assets/pattern-background-desktop.svg')",
        "mobile-pattern": "url('/src/assets/pattern-background-mobile.svg')",
      },

    source link

    and then you should update App.tsx like below

    ❌ delete all this div

    update this line

    with this ✅

    <main className="flex justify-center items-center h-screen bg-[#e0e8ff] lg:bg-desktop-pattern bg-mobile-pattern bg-no-repeat bg-contain">

    and this line

    update with this ✅

    </main>

    because document should have one main landmark instead div.

    i hope these helpful 🙂

    Marked as helpful
    1
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    Hi, nice and clean work. i have a little feedback 🙂

    • you can combine css files into one file for better page performance
    • using one h1 tag on the page is important for accessibility. instead h2
    • i think font-weight should be 600 in this line
    • and no need this padding

    i hope these helpful ✌

    Marked as helpful
    1
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    Hey, nice work. 👍 maybe it could be improved a bit.

    you can use font-family property in body selector. You don't need to add font-family to other classes again and again. After adding font-family to body, you can delete the others.

    for the footer tag to be correctly placed under the main tag, You can add body selector flex-direction: column; property in css file.

    If you want to keep the footer always at the bottom of page... we don't need position:absolute when we use flex. It will be enough to use the flex-grow property in the main tag.

    you can update .attribution class and add main selector like below

    .attribution{
font-size: 11px;
color: var(--White);
height: 3rem;  
margin-top: 3rem;
} 

    main{
flex-grow:1
}

    and update body selector like this

    body {
min-height: 100vh;
margin: 0;

display: flex;
align-items: center;
justify-content: center;
flex-direction: column;

background-color: var(--card_bg_blue);
font-family: var(--Outfit);
}

    And the image url slashes looks incorrect.

    ❌ src="images\image-equilibrium.jpg"

    ✅ src="./images/image-equilibrium.jpg"

    i hope these helpful 🙂

    Marked as helpful
    1
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    hi sefa, nice work 🙂 if you want could be improved a bit.

    to remove the default styling of different browsers, you can use some css reset code top of the your css page like below.

    *,
*::before,
*::after {
  margin: 0;
  padding: 0;
}
img {
display: block;
box-sizing: border-box;
}

    In this way, you will get rid of the unnecessary scrollbar on the page.

    then you can change the image size width:100% for responsive preview. also adding an image alt tag will be useful for accessibility.

    additionally you can use <main> as parent tag instead of div and use one h1 tag for heading.

    1
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    Nice solution. I'm newbie here.

    how do you make an exact match screenshot for Design Comparison. Do you have any tips or suggestions for this? I am comparing my own design with the source design image. it looks same in my computer, but when i create screenshot here, i get a bit of a skewed result. It annoys me 🙂

    1
  • Bilal Türkmen•340
    @bilalturkmenPosted about 2 years ago

    header, main and footer tags are parent elements of a document structure. I think it would be more semantic to use section as children element inside them.

    1