mr-gsay• 640
@GokselSayilan
Submitted
P
Blake Lyons
@blakelyons
All comments
- PBlake Lyons• 300
@blakelyons
Posted
Looks good! I like that you added placeholders to the fields. That should have been in the design mockup! lol
1 - Gerry Lauriant• 760
@GerryL2
Submitted
Feedback Welcome!
PBlake Lyons• 300@blakelyons
Posted
Looks good. One small change I would recommend to remove the jink (the button causing the section jitter) would be to do the following:
- In the dormant state of the button instead of making the button's border =
border: none;make it the same as the hover effect but the border color is transparent:border: 2px solid transparent. - Add a transition for a smoother hover effect:
transition: all 300ms ease;
1 - In the dormant state of the button instead of making the button's border =
- Kian Bago• 60
@kiangraphy
Submitted
What did you find difficult while building the project?
About being the responsiveness of the project
PBlake Lyons• 300@blakelyons
Posted
I didn't really find anything difficult with this project as the card was already basically responsive to begin with.
The one issue I see the most with other solutions, is the lack of any hover effects. In the design, if you look closely, you can see that the black "drop shadow" moves to the right and down slightly. Indicating a hover effect as well as the blog card title, author, and the category "chip".
Overall, your code is relatively clean. I would suggest adding some hover effects.
Marked as helpful
0 - dawidwojtaszek• 350
@dawidwojtaszek
Submitted
PBlake Lyons• 300@blakelyons
Posted
Nicely done fellow sir.
1 @Paulobritto34
Submitted
Very good and simple project, I loved it!
PBlake Lyons• 300@blakelyons
Posted
Looks pretty straight forward. I only have a few suggestions after briefly looking at your projects:
- I would use class names that are more obvious for what they're being used for. For example, instead of
.rodaPe,.blog-cardor even just.cardwould work here. - You're missing an "active" or "hover" state according to the provided design screenshots.
0- I would use class names that are more obvious for what they're being used for. For example, instead of
- mr-gsay• 640
@GokselSayilan
Submitted
PBlake Lyons• 300@blakelyons
Posted
Looks great! I like the fluidity of the UI. The only thing that threw me off a little is the Dark/Light switch. For me, I didn't really see the button at first because it said "Light" and the page was already in "light mode", so I was confused for a second. Maybe it's just me, but I think having it say "Dark" when in light mode and "Light" when in Dark mode would make it more clear that it's going to switch from light to dark and vice-versa.
Marked as helpful
0 - Marek_Ćwioro• 50
@marek4882
Submitted
I would ask for feedback. It will be a great help for improve my skills.
PBlake Lyons• 300@blakelyons
Posted
Overall you did a great job of getting it close to the design. The only things I noticed are:
- The card is missing the
box-shadowthat is in the design. - I would avoid using super generic class names for the text on the card. For example,
.text-1really could be something like.card-titleand.text-2could be something like.card-text. This makes it easier to read the code and understand the purpose of the text. This helps if you're working on a team with multiple developers working on the same project.
Marked as helpful
0 - The card is missing the
- Hi there, I am Juliet• 130
@Jujujulex
Submitted
PBlake Lyons• 300@blakelyons
Posted
Overall, it looks great to the design. The only thing I would critique is the shadows are a bit harder than the design, and the spacing seems a bit narrow compared to the design. I think it does well with a little more room to "breathe".
The only other suggestion I might add for the UI part is to have a breakpoint for both tablet and mobile. It gets a bit weird between those breakpoints.
Your code looks relatively clean. There are a few suggestions I might add that might better apply to "real world" situations:
- Each of the "box" elements I think could be treated as an "article". So the use of the
<article>tag might help in this case. - Your
classnamesare way to specific. You could shave your entire stylesheet down a fraction of what you currently have. You're almost treating classes like ID's. For example:
You have your HTML for the parent container and the tow "rows" like this:
<div class="container"> <div class="daniel-jonathan"> //.... </div> <div class="harmon-patrick"> //.... </div> </div>Inside those you have elements very specific to the "people" featured in the article. This is essentially making your classes unique, like and
id. You're classname.containerworks great, because this is exactly what it is, a container that can be used multiple times...generically.I would recommend the more generic approach when using classnames such as:
<div class="container"> <div class="container__row"> <article class="article article--2-fr"> <div class="author"> <figure> <img src="#" /> <figcaption> <h2 class="name">Jeanette Harmon</h2> <p class="title">Verified Graduate</p> </figcaption> </figure> </div> <div class="detail"> //... <p></p> </div> </article> <article class="article--2-fr"> //... </article> </div> </div>Just my 2 cents... Keep coding my friend!
0 - Each of the "box" elements I think could be treated as an "article". So the use of the
- PBlake Lyons• 300
@blakelyons
Submitted
PBlake Lyons• 300@blakelyons
Posted
I know about the Accessibility and HTML Validation Warnings and Errors. I'm not concerned with that for this project.
0
