Can you help me to improve this page?
Ksenia
@kseniusAll comments
- @AntoooaneSubmitted about 4 years ago@kseniusPosted about 4 years ago
Hi! Good work on the challenge!
I have some advice for you:
1) For such elements as
<div class="users_sup"></div>
and<div class="users_inf"></div>
you could have used CSS pseudo-elements:.users::before { ... } .users::after { ... }
For better code structure organization it would be good to wrap some related elements in
<section>
or<div>
with a suitable CSS class name. Especially sections of the landing that have headings.<section class="cta"> <h2 id="ready">Ready To Build Your Community?</h2> <button class="getstarted btn2">Get Started For Free</button> </section>
I'd recommend you learn more about HTML semantics if you haven't already (I notices that you used
<header>
but you didn't use<footer>
for the page footer).Also, getting familiar with some CSS naming conventions can help you organize your code better.
2) I also noticed you used a lot of ID selectors. Be careful with them. IDs has higher specificity than class and tag selectors and may cause some trouble with applying styles that come from different sets of rules.
3) Not all elements that looks like buttons in a design layout should be
<button>
s in code. More appropriate tag for some of them can be<a>
. When analyzing the design, think if you need a simple button (e.g. a button in a search form or a menu (e.g. hamburger) button) or a link that most likely leads to another page.4) For building pixel perfect solutions and in case you don't have a Sketch file, you can use PerfectPixel extension. It's free and available for Chrome, I'm not sure about other browsers.
1 - @Augs0Submitted over 4 years ago
Always looking for ways to write cleaner and less bulky code, so keen to hear ways I can streamline parts of the projects or perfect the bits I haven't got exactly right. I'm also interested in how to better use ARIA for DIVS that don't really have a purpose.
@kseniusPosted over 4 years agoHi!
- I don't think using
role="presentation"
on adiv
is the correct way to use this attribute. I myself never used it, but what I understood from what I googled about it is that it should be used on elements with semantic meaning to remove that meaning. So, it has no sense to use it ondiv
s.
Also, this attribute affects not only the element it's applied on, but the children of that element too, and you used this attribute on every
div
.In my opinion, the better solution to hide the chat app illustration from assistive technologies would be using
aria-hidden="true"
on the parent element (the element with.phone
class in your case).I recommend you take a look at this article (or the article on MDN) if you want to learn more about presentation role.
- I don't know how much experience you have with CSS, but your
.msg-user-two
class is weird. It's not easy to understand why all those properties is used. The positioning of the chat messages on the right and on the left could have been done with a few lines of flexbox, example below:
.message-exchange { /* your code... */ display: flex; flex-direction: column; } .msg-user-two { align-self: flex-end; }
Much simpler.
- You can build pixel perfect (or maximally close to that) solutions by using PerfectPixel extension for Chrome.
2 - I don't think using
- @DanielGibsonOrchidSubmitted over 4 years ago
Any feedback is much appreciated
@kseniusPosted over 4 years agoHi! Good work on the challenge!
-
There's a problem in the live preview, the tabs and accordion don't work. It seems like it's due to the incorrect path to the JS file.
-
I'd like to recommend you fix HTML and accessibility issues.
0 -
- @zuolizhuSubmitted over 4 years ago
Feedbacks are welcome!
I'm trying to practice components based CSS. That's why my code are kind hot mess LOL.
The background curve image at the intro section are kind tricky. I did it using two position: absolute sections, and then pushing margin top in the next component to make the space for the intro section.
Let me know if you have any better solution!
Thanks!
@kseniusPosted over 4 years agoHi! Good job!
I really liked the hover effects! 👍
The better solution for adding the curve image to the intro section is adding it as a background image via CSS. Thus, you'll be able to position it at the bottom of the section with the
background-position
property and no additional elements withposition: absolute
needed.Here is a link to the CSS Backgrounds reference on w3schools.
As for component approach, I noticed that you used BEM naming. If that is so, I'd like to mention that some of your classes names are incorrect. Here is a couple of examples:
-
.testimonial_card
should be.testimonial-card
, because_
is used for modifiers; -
You have
.profile__avatar
and.profile__image__avatar
. In BEM there can't be an element of an element, but you can nest elements. So, it would be better to name the latter one something like.profile__avatar-image
.
3 -
- @Richard-08Submitted over 4 years ago@kseniusPosted over 4 years ago
Hi! Good work on the challenge!
The filtering works great, but there is one thing you might will want to fix: when clicking on a category tag in a vacancy, the tag is added to the filter panel on the top as many times as it is clicked.
2 - @ayoubarroumSubmitted over 4 years ago
How can I make the button stick to it's place when using position absolute to put it where it is? and is there another way to place it there? The website looks great on my screen but once I post it I see it's not working .
@kseniusPosted over 4 years agoHi!
You can fix the button position by giving
position: relative
to its parent element, so then the button will be positioned relatively to its parent. But what I noticed in your code is that you put the input field in a div , but the button is outside of it. It would be better to group these two elements in the same parent element.Also, setting
width: 39%
on the input field andleft: 43%
on the button is not a good idea. It would be better to setwidth: 100%
on the input andright: 0; top: 0;
on the button, so that your input would always be 100% of it's parent's width and the button would always be on the right of the input regardless of the screen size. And in such case you may need to addbox-sizing: border-box
to the input, so the padding of the input will be included in its width.1 - @marcomedeirosfhSubmitted over 4 years ago
This is my first project out of college and probably you will find a lot of newbie mistakes :P
@kseniusPosted over 4 years agoHi! Congratulations on your first project!
I've looked at the preview and code of your site and I'd like to make a couple of suggestions:
- I'd recommend not to use ID selectors in CSS. Using classes is more convenient and beneficial. Here's the article about the reasons.
I'd also recommend you to learn more about different approaches to CSS, such as CSS naming conventions, for example BEM methodology. It really helps to organize code better, not only CSS but also HTML. Personally my code has become much better with BEM.
- The second suggestion is about responsiveness. Your site is displayed incorrectly between 376px and approximately 1000px. You can solve this by increasing the value of your breakpoint (375px), for example.
1 - @unkletayoSubmitted over 4 years ago
I would love your honest feedback.
Also, I need help on how to give class names I really suck at doing so.
I would also love if anyone could tell me the standard screen sizes for devices
@kseniusPosted over 4 years agoMy honest feedback, as you asked:
First of all, you overuse buttons. You don't need to use the button tag for every element that resembles a button. Particularly in the landing page you've build I would use the button tag only in the subscription form in the footer. Secondly, I'd recommend you to learn more about flexbox, I've quickly looked through your style.css and it seems like you use it incorrectly (e.g. lines 476 and 490). I would also recommend to avoid deeply nested selectors.
As for class names, you need to search for CSS naming conventions, choose one of them and stick to it. For example, there are BEM (Block, Element, Modifier), OOCSS (Object Oriented CSS, if I'm not mistaken) and some other CSS naming conventions of which I don't know much, but Google does :)
2