Skip to content
OverviewSolutions
0
Comments
170
hardy
@hardy333

All comments

  • hardy•3,640
    @hardy333

    Hey, nice solution. Congratulations on completing it.

    Few suggestions:

    • Use cursor: pointer; on button elements and also try to add some :hover effects on them.
    • Do not use width: 100vw; on .container class use width: 100%; instead, this will fix horizontal scrollbar problem which appear on smaller screen sizes when there also is vertical scrollbar, and in general most of the times use percentages instead of vw unit for width values, if there is not some very special case.
    • The way you apply height working just fine, I think. But it is not ideal of course. You will find better ways as you will have more practise.
    • Use some vertical padding on body this will separate your top and bottom cards from edge of the screen on mobile view.

    Good luck and have a nice day, hope this tips and suggestions will help you.

    Marked as helpful
  • hardy•3,640
    @hardy333

    Hi, nice solution, congratulations on completing it.

    Visually everything look super good and nice good job on that.

    But there are some html accessibility issues as you can also see in the report above. There are few things you can do in order to fix those issues:

    • First of all you should always use <main> tag on your page, everything in this tag is considered as a main and primary content, in this case profile cards are main content. So for .container.grid element you can use main tag instead of div tag.
    • Other then main tag every page also need to have h1 tag, in this case there are no heading for which you will use h1 tag, but luckily there are special .sr-only tag which you can use, you can read more about sr-only tags from here, or even try to search about .sr-only class in google to find some good articles and read about it.
    • After using .sr-only h1 tag you should use h2 tags for heading for each profile-card component, not h3, so basically try not to jump on heading tags and try to use them in increasing/order without skipping any of them.

    I think this fixes will resolve all html accessibility issues (hopefully).

    Good luck and have a nice day.

  • hardy•3,640
    @hardy333

    Hi, congratulations on completing this challenge.

    Few Suggestions:

    • Try to apply some box-shadow on button component, also you can make it little bit bigger by applying more vertical padding.
    • In design files On heading component you can notice that phrase - ,,Build the community" and - ,,Your fans will love" are on different lines - there are line break between them, you can achieve that by using <br> tag in the h1 tag. This is a very little detail by I think it is important.
    • Your version of social media icons are little different than on design files, in order to fix them you can use other version of fontAwesome icons which are not filled and then use borders on li/a tag.
    • On mobile screen size, your attribution is too close to bottom screen edge, try to gave some padding on body element or some wrapper container element.

    Hope this suggestions will help you. Good luck and have a nice day.

    Marked as helpful
  • hardy•3,640
    @hardy333

    No criticism whatsoever, You killed it.

    One small thing - on small screen size .container element becomes smaller than screen size and background images does not take whole space on screen, so maybe moving those background images to body element might be better I think .

    Marked as helpful
  • hardy•3,640
    @hardy333

    Hi, nice solution.

    You need to fix some issues on react side:

    • If user filters countries by region or by name from search input and then clicks country in order to see its' individual stats on separate page there are same react errors , you can replicate this error or just check this image out. Looks like you are incorrectly reaching to date and getting undefined but I am not 100% sure.

    • When use navigates from individual country page from main/home page you are making new request for all the data every time. This is not very optimal try to save response data on the first response and then use it over and over again.

    Marked as helpful
  • hardy•3,640
    @hardy333

    Hi, nice solution but why don't you just insert image element in to the <a></a> ?
    It would be the easiest way I guess(if I understand correctly what you are trying to do), if user wants to click it, that activates hover effect anyways. And also you can add :focus state on <a> tag and that will be same as hover state, in case someone only uses keyboard and not mouse/cursor.

    Marked as helpful
  • hardy•3,640
    @hardy333

    Hi, I suggest to move .button-area in to the .card__img div than make .button-area position to absolute and card__img position to relative that way buttons always be positioned and attached with image container div and it will be easy to position it as you wish.

  • hardy•3,640
    @hardy333

    HI, first of all congratulations for escaping tutorial hell xD. For the first time try this is extremely good solution - nicely done.

    Few suggestions here:

    • Add hover effect on button and a tags
    • for .annual-plan__text--change link tag change text color, it is more like purple color on design files than blue, and also lower font size on it.
    • Try to choose better class names for your elements. In general try to make class names as simple and short as possible without compromising your naming system in your case BEM. For example .annual-plan__text--change class name is overly complicated long and even incorrectly set - semantically. Instead try to use something like .annual-plan__link for example this is shorter easy to understand and semantically correct.
    • You are using BEM incorrectly when you are using -- modifier syntax. For example in case of .summary__text, .summary__text--main and .summary__text--sub, summary__text is a div container which wraps all the card text including h1 and p, .summary__text--main is h1. your class name and its modifier --main implies that h1 is some kind of modification of div, which is not correct. So relationship between .summary__text, and .summary__text--main is not that one is modification of other, their relationship is more like one (h1) is child of other(div). So in your case I will use classnames like this: .summary__text for div .summary__text__heading for h1 and .summary__text__paragraph for p. Or you can even use .summary__heading and .summary__paragraph for h1 and p respectively.
    • In general I suggest to read some articles about BEM and you also can watch some tutorials on youtube, for example you can read [this article[(https://css-tricks.com/bem-101/)

    Hope this suggestions will be helpful.

    Good Luck.

    Marked as helpful
  • hardy•3,640
    @hardy333

    Also in your .card-section element instead of using section tags use div tags you are using too many <section> tags in wrong places, semantic html elements are good but you need to use them at correct places...

    Marked as helpful
  • hardy•3,640
    @hardy333

    In order to make buttons stay aligned you can try this styles (as Robin Rowell already suggested):

    .sedan-btn-container{
    margin-top: auto;
}

.card-section {
    padding: 25px 35px;
    text-align: left;
    line-height: 1.5;
    display: flex;
    flex-direction: column;
}
    Marked as helpful
  • hardy•3,640
    @hardy333

    On line 72 and 73 instead of social you should use icon[0]. "social" - variable doesn't exists in your code instead you have icon variable which you call it getElementByClassName("social") - this thing which itself is HTMLCollection object which is almost like a array that is why you need [0] syntax to reach your real .social element.

    Marked as helpful
  • hardy•3,640
    @hardy333

    Hey, nice solution but you have too many accessibility and html validation isses. In order to fix them:

    • Use semantic html elements, for example for .card element use <main></main> element instead of <div></div>
    • Use h2 tag instead of h3 this will fix one issue.
    • Use alt property for all the images
  • hardy•3,640
    @hardy333

    Few Suggestions:

    • Use hover effects on button and link components as it is on design files. For that use :hover syntax on element in css.
    • Make annual-plans component full width of the card..

    Also What do you mean by replacing box-shadows ?

    Marked as helpful
  • hardy•3,640
    @hardy333

    Hey, nice solution good job.

    About your issues:

    • Presented pictures when you download assets files are in .svg format, you should not change this extension name, that is why they don't work properly. In general you can not change extension name from one to another in any type of file(not only for images) and expect to work them properly.
    • For fonts, in browser they are working properly, both of the font sizes loaded and styles are applied on heading h1 tag and another font-family styles are applied on paragraph and button. One thing I suggest is to use one font-family property on body element - this font-family will be the one which you use all the time - it will be your default font-family.

    Other suggestions:

    • Add background change effect on button hover effect, you can achieve that like this: button:hover { cursor: pointer; background-color:hsl(31, 77%, 52%); color: #fff; }
    • Remove overflow: hidden; from body it is not needed.
    • Try to push buttons on the bottom of the cards,
    • Try to remove scrollbars from cards when the view post is very small on height, for that remove max-height property.

    Good Luck.

    Marked as helpful
  • hardy•3,640
    @hardy333

    Nice job bro.. keep it up.

  • hardy•3,640
    @hardy333

    Hi, this is great result for the first time - good job.

    Few suggestion:

    • Make button full width of its container so that it will be the same size as .container__plans.
    • use smaller font-size on paragraph text
    • Button hover state background-color is too gray, try to change it and add box-shadow also.
    Marked as helpful
  • hardy•3,640
    @hardy333

    Hey, nice solutions - good job.

    But try to make it responsive on small screen sizes;

    also add border: 2px solid var(--light-gray); on buttons, not only :hover state but also default state it will fix content shifting problem while hovering buttons.

    Marked as helpful
  • hardy•3,640
    @hardy333

    Nice solution, good to see that you don't have accessibility issues,

    One thing I suggest is to implement hover effects on buttons.