@kkalvaa
What challenges did you encounter, and how did you overcome them?
At first, I was very confused about setting shadows, but later I solved it by referring to some other people's code.
What specific areas of your project would you like help with?
None but any advice or suggestions are much appreciated.
@kkalvaa
Posted
Good updates, here are acouple thoughts on the new stuff:
1: Since this is the only thing on the page it is technically correct to use H1. However, this component would be a part of a page and as such the card title should probably be H2 or H3, depending on the page hierarchy. But again, it is technically correct on this single page to use H1.
2: A big point of BEM is to keep specificity as low as possible. Instead of ".card .card__avatar" (0 2 0) it should just be ".card__avatar" (0 1 0). The lower you keep specificity the easier it is to override it later. If you were to have a ".card__avatar--square" class, or --big/--small/etc., you would now have to also up the specificity to 0 2 0 in order to override ".card .card__avatar". This quickly leads to a specificity war and you'll find yourself overqualifying your selectors more than you need to. It doesn't matter too much in a css-file of this size, but it does matter when you scale up, especially when you add more people to a project. In my experience almost all problems in css is a direct result of specificity problems.
Marked as helpful
What challenges did you encounter, and how did you overcome them?
At first, I was very confused about setting shadows, but later I solved it by referring to some other people's code.
What specific areas of your project would you like help with?
None but any advice or suggestions are much appreciated.
@kkalvaa
Posted
A couple thoughts:
1: You could consider adding a transition to the hover effect on the card itself. Having the shadow instantly move to its hoverstate is a bit jarring. Try looking into transition
2: You should look into BEM style naming for your classes. Instead of "card" and "card-text"/"card-avatar" it should be "card__text" and "card__avatar". You're halfway there already, might as well actually adopt the actual naming system. A good primer
3: You shouldn't use H5 without previously having used H1, H2, etc. on the page. H5 means heading level 5, which means it should be a subheading of four previous levels of headings. Unless you're marking up a scientific paper there is a very high chance you will never need to go below H3 on a normal web page.
3B: Learning shouldn't be a heading at all, it is a tag, not a heading. Same with the date, not a heading, but should go in <time></time>. "HTML & CSS foundations" is the actual title/heading in this design.
4: The Sass looks good, I would look into replacing Sass variables with css custom properties these days as they are more flexible.
Marked as helpful
@arthurbicego
Submitted
I had some difficulties simplifying the code, especially considering responsiveness. And I had a lot of difficulty aligning some elements. It would be great to have feedback on coding optimization and whether I wrote the responsiveness appropriately.
@kkalvaa
Posted
Couple things:
1: Don't absolutely position content I noticed that you're using "position: absolute;" to put the component in the middle. As a result both <body> and <main> end up "empty" and you can't reuse the component elsewhere. You should instead use display: flex or grid on the parent container and justify and align to center. By doing so you make a more reusable component that doesn't dictate its position by itself, but the responsibility of positioning is moved to the parent container, where it belongs.
2: Don't use id selectors. In css it is considered bad practice to use id selectors over class selectors (or similar). The reason is specificity, ids have a MUCH higher specificity than classes and aren't reusable. Even if you only intend to target one single element you can still do so with a single use class, but you regain the advantages of having lower general specificity. Also, if the component where to be used twice on a page you wouldn't run into conflicts having to identical ids on the same page.
3: Be careful with absolute values. You're using absolute widths and heighs a lot in your code, that can lead to problems with responsiveness. Usually I find it better to not specify a height or a width, but let the element use the available space, or use min-width/max-width, etc. My guess is that most of your responsiveness problems comes from absoulte values.
4: Don't use magic numbers. This is an extension of the previous, but separate issue. Wen you specify a value, like 55px height. That only works because it happens to be the right value right there, but if there's an ever so slight change in the rendering of the page, say the user zooms in a little bit, things can stop being just right. Read this for more info: https://csswizardry.com/2012/11/code-smells-in-css/#magic-numbers
5: Don't repeat things in media queries you're not actually changing. You should only write code you're actually changing inside a media query. For instance, there's no point in setting the font-family on the *-selector to the same value as you have it normally. That only leads to more code and less maintainable code. Only write things that actually changes.
6: Use the :root selector instead of the all selector for universal styles. font-family, font-size, etc. should be put on :root and then cascade down to everything instead of being explisitly set to everything. It'll be easier to override that way.
Marked as helpful
@timo9939
Submitted
@kkalvaa
Posted
A couple of things:
1: Do not use <body> as your component This rating component should be able to be placed on a page somwhere, you can't put a <body> inside of a <body>, unless you use an <iframe> (which you really shouldn't for this). The component should be a <section> or a <div> or possibly an <aside>.
2: You forgot to remove the default border on the button Try "border: 0;" in your css to remove default borders on buttons.
3: The text is the wrong typeface The design is clearly a sans-serif, the finished solution is using serif text.
4: Try adding a transition to the number buttons for the colour change. Transitions are simply easier on the eye than abrupt changes. My favourite: "transition: all ease-out 200ms;"
5: SuccessMessage text goes outside of the main box. Since you're spesifically setting a width on the success message which is larger than the container the text bleeds out of the box. Generally you'll never need to add a spesific width to any child element of a component (at least if you don't hate your life and want to make everything harder than it has to be).
@JohanXTheKing
Submitted
@kkalvaa
Posted
I like how you're using grid for this and that you're changing the amount of columns based on viewport width. However, on some widths you're letting the columns get a bit small which leaves the 1 column cards with a very narrow text. You should look into Una Kravets RAM grid: https://codepen.io/una/pen/oNbvNQv
The cards really shouldn't be figures, they should be divs or article. <figure> is meant for images and stuff, but this content is text based.
Marked as helpful
@sukanyagurav
Submitted
@kkalvaa
Posted
I like the animation, but the timing is a bit slow. It feels like it takes too much time to animate in all the elements.
Marked as helpful
@wiktor-nowak
Submitted
Can you address some way to better organize my CSS code to help keep it less bushy and simpler to modify?
@kkalvaa
Posted
I would recommend lookin into two things for your CSS:
1: Learn BEM and ITCSS. BEM is a very good naming structure to make CSS easier to manage and ITCSS (Inverse Triangle CSS) is a principle for organizing CSS.
2: Learn Sass. With Sass you can split your css into multiple files and import them into one big document on compile.
Both things will help making your CSS more manageable.
Marked as helpful
@KathrynDavies123
Submitted
@kkalvaa
Posted
You do need to use IDs for for=…, that is true, but you do not have to attach your css to the ID, even if you do have an ID on an element. You can just add a class as well and hook your css onto that instead of the ID and you'll lower the specificity dramatically. Classes will never be able to override ids without !important, except in an earlier version of a browser (I forget the exact one) where a bug let you chain 256+ classes to override an ID. This has since been patched and will no longer work. If you add classes to your labels, and hook your css to those instead of the IDs, you'll most likely find that you don't need !important anymore.
You are however right that in the HTML semantics is important and you should be using IDs to connect the labels with the input elements. CSS however isn't semantic and carries no semantic value, you can name your selectors whatever you want and it won't make any difference. Good naming is for you, the developer (and other developers), not browsers and assistive technology.
I don't really know why it didn't work without IDs, but most likely you have conflicting css overwriting that specific part of your code. I'm guessing it is a specificity problem, an inheritance problem or a order in code problem. You'll have to debug to find the culprit.
Tips on debugging tricky CSS:
One last tip: line-height doesn't require a unit, but will work as a relative measure. Meaning, you can write line-height: 1.5; and you'll have a line height of 1.5 regardless of what the font size is, but if you specifically set a unit, say line-height: 1.9rem;, the line height will always be that size even if you change to a larger font size later/elsewhere.
Marked as helpful
@KathrynDavies123
Submitted
@kkalvaa
Posted
Some comments on your CSS:
It is generally better to use class selectors over id selectors as classes have a much lower specificity and is reusable. So instead of #billamount you should use .billamount. classes can still be single use.
About your naming of classes: The common CSS naming structure is use hyphens to separate words for readability: .billamount becomes .bill-amount, #numberofpeople becomes .number-of-people, etc.
"Magic numbers": I noticed that you've used a lot of exact pixel sizes for things, instead of relative sizes, such as ems or rems. Most of the time it is better to not use an exact number and instead use relative sizes. Sources:
You also have a lot of repetition where different classes have the same content. Example: *#billamount { background-image: url("../images/icon-dollar.svg"); background-repeat: no-repeat; background-position: center; background-position-x: 20px; }
#numberofpeople { background-image: url("../images/icon-person.svg"); background-repeat: no-repeat; background-position: center; background-position-x: 20px; }*
The only difference here is the background image URL. It would be better to use one common class for both and then one specific one for each where you add the background image. Example:
*.icon-button { background-repeat: no-repeat; background-position: center; background-position-x: 20px; }
.icon-button--dollar { background-image: url("../images/icon-dollar.svg"); }
.icon-button--people { background-image: url("../images/icon-person.svg"); }*
while you don't gain anything in terms of code length, you do gain maintainability. This way if you need to update something on .icon-button you only update it once instead of multiple times, which leads to lower chance of errors/typos.
DON'T USE !important !important ruins specificity and leads to a lot of problems in itself. Most of the time you can fix the issue with a refactor.
@romenter
Submitted
I had some problems to face in this challenge that have nothing to do with my dumpness (hehe): There some tips:
If you had some tip to me, please, do it. I am learning, any advice is so f**** welcome
Remember, you study for your future, have a nice code day ❤
@kkalvaa
Posted
I'm not sure what happened, but when I open the solution in both firefox and chrome it is in a broken vertical layout. Could be just me though …
Regarding the email validation issue: Browsers will validate emails for you if you use <input type="email" validate> on the input field.
Marked as helpful
@kinjames
Submitted
I used firefox browser to open it and the view was different from chrome. I am having a little tough time trying to make it responsive. Any feedback on how to solve these issues are welcome and will be much appreciated. Thank you.
@kkalvaa
Posted
Parts of your responsive problem stems from not using "flex-wrap: wrap;". If you add wrapping to your flex container and "flex: 1 1 200px;" on the children things will instantly be more responsive.
However, this is a much better fit for grid than flex, and I strongly recommend you try it out.
Marked as helpful
@AbdelrahmanTolba
Submitted
-it not difficult
@kkalvaa
Posted
For responsive images you should look into srcset instead of having two <img> and hiding one with css. When you hide images in css the browser will still load the image, but only show you one. In other words you end up using more bandwidth than if you just sent over the big one and scaled it down.
Instead of "card__left" you should call it "card__image" or similar. Since the image isn't on the left in mobile view "card__left" doesn't make sense anymore, but "card__image" always will.
Marked as helpful
