@JanAbe
Submitted
@tburette
@JanAbe
Submitted
@tburette
Posted
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?
@IamVictheCG
Submitted
@tburette
Posted
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)">
@WaMungai
Submitted
@tburette
Posted
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>
,...
@kaamiik
Submitted
I would appreciate it if you could take a look at my code and give me feedback.
@tburette
Posted
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
@KatMahn
Submitted
Please comment on how my code is structured and if there is a way I could have done it differently
@tburette
Posted
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:
<div class="attribution">
could be a <footer
<h3>
for the link/button. Isn't there an HTML that would be a better fit?In the CSS:
@media
).Marked as helpful
@tburette
Posted
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
@tburette
Posted
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 :
grid-column: 1 / -1;
to make an item span all the columns not matter their number. Smart.Marked as helpful
@tburette
Posted
Hello, Very good clean solution! There are a lot of things to take inspiration from :
Personal preference : instead of grid-column: 1 / 3;
, you could use grid-column: span 2;
.
Marked as helpful
@ThinYuShwe
Submitted
@tburette
Posted
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
@Diozizo
Submitted
@tburette
Posted
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
@Ghosh-95
Submitted
@tburette
Posted
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)!