Skip to content
OverviewSolutions
5
Comments
11
Thomas Burette
@tburette

All comments

  • Thomas Burette•190
    @tburettePosted 10 months ago

    Nice result and nice code!

    You use BEM. In your header there are classes for the modifiers but not the block. You have class="header__title but no class="header".

    To do the layout of the cards in the desktop layout you used a grid with three columns and four rows. What I did was that I had three columns and two rows. First and last card span the two rows. Middle two cards occupy their own row (+ some adjust toward the center). I find your solution simpler than mine.

    I also noticed that to have an item span one column you used grid-column-start: 1;. I didn't you could do that. I'm used to do grid-column: 1 / span 1;.

    I'm curious. Where does the css reset that you use come from?

    1
  • Thomas Burette•190
    @tburettePosted 10 months ago

    Nice solution @IamVictheCG !

    Here are a few remarks.

    Your html could be more semantic by using <main>, <section>, ...

    I find your word spacing on the titles a bit too big but that is a purely stylistic choice.

    I see that you use separate css files for desktop and mobile. Interesting. Did you know that you can import css files conditionally?

    css: @import url(a.css) (min-width: 800px);

    html: <link rel="stylesheet" href="a.css" media="(min-width: 800px)">

    0
  • Thomas Burette•190
    @tburettePosted 10 months ago

    Nice solution. The CSS is simple to read and does the job in few lines!

    Instead of .btn:hover{} you could do .btn:hover, .btn:focus. It is better for accessibility : it makes the CSS works if people use keyboard navigation (tabbing).

    You could use more semantic elements in your HTML : <main>, <footer>, <section>,...

    0
  • Thomas Burette•190
    @tburettePosted 10 months ago

    Nice solution!

    Your CSS could follow BEM more. You did <div class="card sedans"> but you could have done <div class="card card--sedans">. See Why the modifier CSS classes are not represented as a combined selector?

    When the button becomes active the card moves a little. Is it intentional? It could be a neat effect. If it is not : you add a border on :hover/:focus. It causes the layout to change.

    Marked as helpful
    0
  • Thomas Burette•190
    @tburettePosted 10 months ago

    The result looks nice. It doesn't exactly match the given design and that's okay! (Adjusting a layout to closely match the given challenge is a lot of time for little payoff.)

    In the HTML:

    • There is an unclosed HTML comment at the end
    • <div class="attribution"> could be a <footer
    • you used an <h3> for the link/button. Isn't there an HTML that would be a better fit?

    In the CSS:

    • When the viewport is just above 830px part of the content is not visible and there is horizontal scrolling. There seems to be some kind of mixup in your media queries (@media).
    • You could implement the effect on the button when it is active (see design/active-states.jpg)
    Marked as helpful
    0
  • Thomas Burette•190
    @tburettePosted 10 months ago

    Nice solution. You used place-content which I didn't know about. It is a shorthand property for align-content and justify-content.

    There is a smooth transition when the breakpoint is it : the vertical space between the items within each of the three cards smoothly increase/decrease. I looked at the code and it is caused by :

    .button {
  transition: all 0.4s;
}

    At first I didn't understand why transition: on a button affect the space between the items of a grid. By fiddling around I realized it is the margin-top property of the button which transitions.

    There is a bunch of CSS that is unused in the final solution : in .button {} there are two border:. .orange{}, .green{}, .cyan{} are unused,..

    Marked as helpful
    0
  • Thomas Burette•190
    @tburettePosted 10 months ago

    On a very wide and small vertically viewport some of the content is clipped. This is due to :

    @media (min-width: 985px) {
  body {
    height: 100vh;
  }
}

    It really has to be an extreme aspect ratio to happen.

    Like last time, there are several things that I liked in your solution compared to mine :

    • I like the use of figure + figcaption. I didn't think of it.
    • I completely skipped the background pattern. It really adds that 👌.
    • I noted the grid-column: 1 / -1; to make an item span all the columns not matter their number. Smart.
    Marked as helpful
    2
  • Thomas Burette•190
    @tburettePosted 10 months ago

    Hello, Very good clean solution! There are a lot of things to take inspiration from :

    • custom properties to define color fonts with well organized naming
    • small negative letter spacing
    • I noticed many things that you did right for the button that I did wrong. Your button goes down when you use it, not up. The shadow effect works not just on :hover but on :focus as well.
    • Good implementation of BEM. It's weird to see it used for such a 'major' part of a page that is an entire section. However since the sections are simple it works well here. I imagine if sections were to evolve into something more complex the use of BEM on section would break down.

    Personal preference : instead of grid-column: 1 / 3;, you could use grid-column: span 2;.

    Marked as helpful
    0
  • Thomas Burette•190
    @tburettePosted 10 months ago

    Hello, You use a lot of div where you could use semantic elements instead. For example yo ucould use <main> instead of <div id="outerdiv">. <section> instead of <div id="firstpar">, ...

    Your design doesn't work well when it is above 375px but still narrow. For example at 450px the "Why us" section is very narrow and the text hard to read.

    An advice to fix this is to avoid arbitrary breakpoint such as 375px. Instead : choose the breakpoint based on the design insteadof a specific spot. To do that widen your mobile layout and determine when it start to become worse than the desktop layout. Pick that point as the transition point.

    Marked as helpful
    0
  • Thomas Burette•190
    @tburettePosted 10 months ago

    A fine solution. The result matches the challenge.

    The html/css does the job. The remarks below are more nitpick than anything else.

    In the html I noticed you put a random amount of newlines between the container and the attribution.

    Instead of margin-left: auto; margin-right: auto; you could have done margin-inline: auto;. Another possibility would have been /* top | left and right | bottom */ margin: 0 auto 1.3rem; but that's not necessarily very readable..

    Having custom properties start with an uppercase is unusual.

    Marked as helpful
    0
  • Thomas Burette•190
    @tburettePosted 10 months ago

    Nice solution.

    Straightforward html and css that is not hard to read.
    It matches the input. You made me realize I didn't put a shadow on mine.

    Auto marging to center is fine (for simple situations like this one)!

    0