Skip to content
OverviewSolutions
3
Comments
46
Alex
@al3xback

All comments

  • Alex•660
    @al3xback

    Hi Anosha,

    I think this structure is not well optimized when have long content. (https://prnt.sc/24lvdv1, https://prnt.sc/24lvvu0)

    Marked as helpful
  • Alex•660
    @al3xback

    Hi Christopher,

    I think we dont need to add width 80% on .card class. Cos this will create empty space. Attached screenshot for a better view. (https://prnt.sc/24lspqh)

  • Alex•660
    @al3xback

    Hi AleksandarV91,

    I see double scrollbar here, https://prnt.sc/24gxp6o

    Looks like there is a structure problem

  • Alex•660
    @al3xback

    Hi Quincy,

    It would be good if we can set position: relative to each grid items, not in .Grid class. So this will lead to a better placement of the content.

    Marked as helpful
  • Alex•660
    @al3xback

    Hi Wildmarks,

    I see that the color (#0c1729) is not working cos it's overridden by children's background-color (#15273f) on class contentContainer.

    The css concept is actually each children can overwrite/change the inherit value from the parent.

    Marked as helpful
  • Alex•660
    @al3xback

    Hi Jane,

    When i tried to open on smaller desktop height, i could not scroll to see the overall content, looks like it's affected by overflow: hidden

  • Alex•660
    @al3xback

    Hi Emiliano,

    For this case, it would be good if we use fixed value instead of percentage for max-width on container class. If we look on larger desktop or simply zoom out then we will see the difference.

    I would change to this:

    .container {
    max-width: 1200px;
    padding: 3% 5%;
    margin: 0 auto;
}
    Marked as helpful
  • Alex•660
    @al3xback

    HI Mark,

    My feedback are as follows:

    • for class nft-photo, using flex properties is unnecessary cos we dont have content inside.
    • for class ETH and time, we dont need to use flex-direction: row as its default value for flex.
    • it's unnecessary to add color property on class line, it's alr handled by border.
    Marked as helpful
  • Alex•660
    @al3xback

    Hi Achref,

    It would be good if we can move social and copyright to footer instead of section.

    Marked as helpful
  • Alex•660
    @al3xback

    Hi Achref,

    By using place-content, behind the scene, we are adding special styles to grid-container (grid-template-columns: min-content; and grid-template-rows: min-content;) which make each grid-items shrinks so we can place them easily according to the value of place-content.

    Those additional styles won't work if we assign fraction unit value to grid-template-columns. Fraction unit provide grid-item the ability to occupy existing space (similar to grow in flex) and this will make grid-container difficult to manage position of grid-items.

    Hope this clear :)

    Marked as helpful
  • Alex•660
    @al3xback

    Hi Achref,

    I think for this case we dont need to use place-items. This will break the layout if the grid-item doesn't have paragraph element. Attached screenshot for this case. (https://prnt.sc/23dredg)

    and for place-content, it doesnt have effect if we combine with fraction unit(fr) on grid-template-columns (cos usually only work for px or auto).

    Marked as helpful
  • Alex•660
    @al3xback

    Hi AliAbuhumra,

    I would recommend using margin-top instead of translateY on .quotes__item, by using this will allows the parent's height equal to children height (including margin).

  • Alex•660
    @al3xback

    Hi Natasya,

    My feedback is:

    • It would be better if we set image on header to be a background-image instead of img tag. The reason is:
    a) this is just static image and never changing too often (even no need for CMS setup for future).
b) we are not going to use picture element for better rendering.
c) less code.. haha
    • on main, changes will be:
    main {
    ...
    flex-wrap: wrap; //remove
    align-items: center; //remove
}

    we dont need align-items: center cos each .list element has width property 100%, then this has no effect :) and we dont need flex-wrap: wrap cos we dont set exact height to it.

    Marked as helpful
  • Alex•660
    @al3xback

    Hi Brivan,

    I think it is better to set grid-template-columns by using fraction unit (1fr). currently the grid items dont fully cover the grid container, leaving minus 1% on the right side.

  • Alex•660
    @al3xback

    Hi Noemi,

    It is better to change height to min-height on body. try to zoom-in until the browser height smaller than the content, and see the difference. min-height allows better view to see full content.

    Marked as helpful
  • Alex•660
    @al3xback

    Hi Eshan,

    My feedback is:

    • step 1: remove place-content and background properties(except: background-color) from body and add some grid properties.
    body {
   ...
   place-content: center; //remove
   background-color: //keep!
   background-image: //remove
   background-position: //remove
   background-repeat: //remove
   grid-template-columns: 1fr auto 1fr; //add
   grid-template-rows: 1fr auto 1fr; //add
   overflow: hidden; //add
}
    • step 2: add 2 new html elements, place it before main, the structure will be like this.
    <body>
   <div class="decoration-bg decoration-bg-1"></div>
   <div class="decoration-bg decoration-bg-2"></div>
   <main>...</main>
   <footer>...</footer>
</body>
    • step 3: add styles to newly added elements.
    .decoration-bg {
    position: relative;
}

.decoration-bg-1 {
    grid-area: 1 / 1 / 2 / 2;
}

.decoration-bg-2 {
    grid-area: 3 / 3 / 4 / 4;
}

.decoration-bg-1::before,
.decoration-bg-2::before {
    content: "";
    position: absolute;
    width: 980px;
    height: 980px;
    background-size: 100% auto;
    background-repeat: no-repeat;
}

.decoration-bg-1::before {
    bottom: 0;
    right: 0;
    background-image: url(./images/bg-pattern-top.svg);
    transform: translate(20%, 20%);
}

.decoration-bg-2::before {
    top: 0;
    left: 0;
    background-image: url(./images/bg-pattern-bottom.svg);
    transform: translate(-20%, -20%);
}
    • step 4: add styles to main and footer.
    .card {
   ...
   grid-area: 2 / 2 / 3 / 3;
}

.attribution {
   ...
   padding: 12px 0;
   grid-area: 3 / 1 / 4 / span 3;
   align-self: end;
}
    Marked as helpful
  • Alex•660
    @al3xback

    Hi rizahoemae,

    In my opinion, using the opacity property to reduce the contrast of a color is not a good idea, because there is a condition where if in future we want to add an image, icon or other element inside that element, then the newly added element will also be affected by the opacity.

    It is better if we set through tailwind text-opacity:

    from:

    <div class="text-white opacity-50 text-[14px] mt-2">Our Equilibrium collection promotes balance and calm.</div>

    change to:

    <div class="text-white text-[14px] mt-2 text-opacity-50">Our Equilibrium collection promotes balance and calm.</div>

    This approach will only inherit color.

  • Alex•660
    @al3xback

    Hi Quang,

    Would be good if we dont set width on each grid item (in this case on .card class) if we use grid-template-areas.

    let the parent controls the width of its children using grid-auto-columns: 1fr; or grid-template-columns: repeat(5, 1fr);

    Marked as helpful