|
|
|
<path id="Path_4" data-name="Path 4" d="M270.939,38.414H260.875a1.516,1.516,0,0,0-1.516,1.516v5.693a1.515,1.515,0,0,0,1.516,1.516h3.58l-.579,2.335a.331.331,0,0,0,.523.343l3.486-2.678h3.053a1.516,1.516,0,0,0,1.516-1.516V39.93A1.516,1.516,0,0,0,270.939,38.414Zm0,0" transform="translate(-245.892 -36.419)" fill="#88dbfd"/> |
adwaith
Hey! Have made several comments, please resolve them and commit the next set of improvements.
Make the font family “Oswald”. Lowercasing, and camel/snake casing gives an impression of variables. This isn’t a variable, this is a text fragment, treat it as such
Weird folder structure - why would you put the TTF file inside a static folder?
Inconsistent quotation marks. Either use single quotes or double quotes throughout your CSS, HTML, or any language.
Too generic - there’s no guarantee that there’s only one header in this page. (In fact, it won’t be just one header; the card has a header too).
Use a more specific selector, such as
header#page-title h2instead.Good idea, stick with rem units. They’re better than px.
Space after the
":"missingPercentage values for padding can be confusing - especially for the vertical ones. The vertical padding will change with the width of the element, that doesn’t make much sense. I’d stick to padding in pixels here, it doesn’t need to scale.
Not sure, why it’s confusing and how does vertical padding affect width? It’s the height that get’s affected by vertical padding right?
Also I was searching about problems with precentage padding and bumped into this: https://css-tricks.com/oh-hey-padding-percentage-is-based-on-the-parent-elements-width/
This got me confused even more.
Space between commas, please. Almost all linters in all languages recommend this.
Poppins hasn’t been defined - there’s no TTF file it’s linked to. Plus, lowercase and inconsistent quotation marks
Could have given a better name. Especially if there is more than one kind of card in the page. Use this rule of thumb - the class names should make sense even if you don’t have any concept of CSS. Here I’d have gone with
.job-description-cardAlso, space after the CSS selector, before the flower bracket. This is a running issue everywhere, I won’t point it out for the other selectors - I assume you can fix them all with this reminder.
This is something you want to scale with the screen size. Should be dealt with percentage width.
There are very few situations when you’ll need to specify the height of an element containing text. This isn’t one of them. Keep the height variable. Otherwise this won’t be responsive AT ALL.
I found mulitple ways to do this.
Also want to understand how setting parent element’s min height and height makes child height variable?
Can be converted to shorthand:
padding: 28px 20px;Can also be converted to shorthand:
margin: 40px auto 0;Doesn’t make sense to specify the
leftandrightproperties for relative positioning. What are you trying to achieve with this?yeah that’s right. I’m not sure why I added that. I was probably trying to justify it in the center.
Group similar properties together. This should have been with
margin-leftandmargin-right(ideally in a shorthand)Absolute no-no - do not specify stylistic properties in a class name! Your HTML shouldn’t care about style at all, it should only care about structure. This should have been a descendant of the class name above. I would have added these rules for
.job-description-card header. Don’t rely too much on giving a class name to every element - some of them don’t need it.This is the opposite of what you want - You know exactly how much space you want the designation image and share icon to have, and you want the job description title and subtitle to fill up the remaining space. That’s handled by
50px 1fr 30px(or it’s rem equivalents, if you want to use rem units)There’s no need to limit the no. of rows by defining
grid-template-rows. Plus, you don’t even have two rowsYou have no room for
justify-itemsto even apply, if you’re using the fr units to populate thegrid-template columnsand it adds up to 1. All the free space has been distributed to your children, so there’s no room for alignment at all, it’ll be jam-packed. This should have beenalign-items: centerat best.Since it’s only the share icon that’s vertically centered, though, even that isn’t a good idea.
You’re using this to put space between the header and the separator. Use
margin-bottomfor this. But in this case, though, it’s better to remove this entirely. It’s the separator’s job to separate elements; add the margins to the separator.Again, no - this makes your HTML unreadable. If you want the first grid item, you should’ve used
.job-description-card header > :first-childinstead.But this doesn’t even warrant that! Just give it a proper class name!
.job-designation-avatarDefining rows and columns is completely useless - the children of a grid element flow naturally, left to right, top to bottom. This is only used when you’re doing convoluted stuff with grids. Remove entirely.
Better to align the share icon to the center, than this to the baseline.
Specifying a width to the child of a grid element is just weird. Just specify the grid to make this column 50px wide.
You’re specifying such weird padding because of the image being asymmetrical - not a bad choice, but it’s better to specify this in the image element, since it’s the image’s problem. Especially because we don’t know if every job description will have the same asymmetry - this specific set of “magic numbers” will only apply to this one job description card.
Better to make it a percentage of the parent. Try to make the design as responsive as possible. Relative units first - if that doesn’t work, then try absolute units.
Bad name again.
Again, useless properties unless you’re doing something fancy and non-standard with grids
You’re trying to manage the gap between grid children with your padding here. That’s an anti-pattern. Use the
grid-gapproperty. Padding should only be used if you want to put space between your border and content.Not necessarily a CSS border; I’m speaking conceptually. If you want to create space between two things, use margin (or in cases like this, grid-gap). If you want to create space within something, use padding.
Usage of the direct descendant isn’t a bad idea. Use it with a better name next time, though.
Not sure what you’re trying to accomplish with this - I thought this should just have a padding bottom, and use either a grid-gap, or padding within the grid itself to create space around the other sides of the job designation heading.
Space after colon!
Letter spacing in px, while haveing a font size in rem doesn’t mix well. Keep it consistent. rem is a better idea.
Consistent, please. Don’t use lowercase in some hex codes, and uppercase in others. In fact, don’t use hex codes at all, they don’t give any hint to what the color should look like. Use
hsl()instead - that intuitively gives an idea of what color to expect just by looking at the numbers.This whole ruleset can be removed, all it’s properties are redundant.
A bit misleading - there’s nothing special about the top border. You’ve given it a height anywa, give it a background color of
hsl(0deg, 0%, 95%)or somethingYou can also have margins on the separator
Get rid of this entire ruleset. This shouldn’t be a grid at all. There are multiple line items, each with an image and a text. Style the line item - make it a grid or a flexbox. (with fixed width image and a free width text)
Not only do you have to change this every time you add a line item to a card, you’re also guaranteeing that each card will have the same number of line items. Stupendously bad idea. Props for using
repeat(), though.Better. Fixed width image, and free width text. But it’s better to use grid terminology.
grid-template-columns: 0.2fr 0.8frshould be fine. But don’t use this for the overall grid, use this (or something similar in flex) to each line item.Redundant if you provide margins on the separator.
Everything from this point...
... upto this point needs to be re-written.
If you start repeating your code, it’s a sign that you’re doing something wrong. Rule of thumb - code can be repeated twice, but if you’re repeating the same code thrice, you’re doing something wrong.
Define a
.job-description-bullet-pointclass which is a grid that has an image and text. Style those accordingly, and re-use those styles by applying the same class to multiple elements. You’re actually using these classes as though they’re IDs.Unnecessarily convoluted. Just replace with
border: 1px solid hsl(<some-value>);SVGs usually have an ID. Get rid of it and replace it with a class name. Otherwise there’ll be trouble when you re-use the same SVG multiple times in the DOM. (Apply for all included SVGs)
Same goes for these paths. Just get rid of the IDs. (Apply for all included SVGs)
Better to make this a section, or even better, a
<main>elementUse
<section>when you think it’s something structural that the end user will think of as a separate element. Use<div>when you’re creating an element purely for style reasons, nothing to do with structure. This is somewhat subjective, but the idea is that<div>has less meaning than<section>In this case, though, an
<article>tag might be a better fitI’ve harped enough about the names in the CSS section, I hope - I won’t mention it beyond this point, recall said comments when you see a class name.
It’s a good idea to use srcset when you have multiple images, but there’s really no point to it when you have a single image. Especially when you’re using a vector based format like SVG rather than a raster one, you can just use the
srcattribute.It’s a good idea to use srcset when you have multiple images, but there’s really no point to it when you have a single image. Especially when you’re using a vector based format like SVG rather than a raster one, you can just use the
srcattribute.Misspelling. Plus, You aren’t trying to describe the image - you’re trying to describe it’s meaning. In this case it should’ve been “Developer”
Not sure if a
<p>is justified here - this is a sub-heading. Use<h4>or below instead.Same comment as above - srcset is mostly wasted on SVGs which are independent of display size
No need to specify things like “image” or “icon” in the alt tag of an
<img>element. We know it’s an image, just describing the meaning behind it is fine. Remember, this is used when images don’t load, or by screen readers for the blind, etc. They won’t benefit from knowing it was an icon.All the problems with this layout is covered in the CSS file review. Please go through it and change all this below accordingly.
评审人