Skip to content
OverviewSolutions
11
Comments
140

CyrusKabir

@CyrusKabirnowhere1,885 points

☺

Latest solutions

View more solutions

Latest comments

  • CyrusKabir•1,885
    @CyrusKabir

    Hello Elain, you did a good and clean job on this challenge as always. really good. I just want to know why you don't use some good features in sass when you are using sass. I mean using some loops for generating custom props or utility classes or some @mixins and placeholders.

  • CyrusKabir•1,885
    @CyrusKabir

    Hello @pjortega0225, for centering things I highly recommend read this two articles :

    Marked as helpful
  • CyrusKabir•1,885
    @CyrusKabir

    Hi @dannxvc, you did a good job on this challenge. your card have some problems both in code and accessibility.

    1.Accessibility: at first I just try to test accessibility with just tab key and I didn't know for radio groups we should use tab + arrow keys. one hidden and weird bug is when some one want select rating number 1 at first with just keyboard it's hard I mean first tab key just can select our radio groups then we should iterate with arrow keys and if some one hit tab key at first for rating number 1 and then hit enter in thank you card there is no rating selected

    2.CSS: you have some duplicates code in rating buttons implementing. like adding pseudo elements to label for just creating hover effects and for button itself. you can do all of them in label itself. one another duplicates is two different rules for .options label and both of them have position: relative. I changed some duplicated codes and here is the result :

    .options input[type="radio"] {
    opacity: 0;
    position: absolute;
}
.options input[type="radio"]:checked + label {
    background-color: var(--primary);
}
.options input[type="radio"]:checked + label span,
.options input[type="radio"]:focus + label span{
    color: var(--white);
}
.options input[type="radio"]:focus + label{
    outline: rgb(59, 153, 252) auto 5px;
}

.options label:nth-of-type(1){
    margin-left: 0;
}

.options label:hover{
    transition: .3s ease-in-out;
    background-color: var(--gray);
    color: var(--white);
}

.options label{
    position: relative;
    display: inline-block;
    height:  3rem;
    width:  3rem;
    background: var(--blue-light);
    border-radius: 50%;  
    padding: 0.5rem;
    cursor: pointer;
    justify-content: center;
    margin-left: 1rem;
}

span{
    position: absolute;
    top: 50%;
    left: 50%;
    transform: translate(-50%, -50%);
    z-index: 1;
    color: inherit;
    font-size: 0.8rem;
}

    I still haven't found a solution for accessibility problem. but at all you did good job on having good accessibility with those radio buttons also I learned about how we should interact with radio buttons with keyboard. hope I could help you. good luck. ☻

    Marked as helpful
  • CyrusKabir•1,885
    @CyrusKabir

    Hi Elain. you did a clean and good job on this challenge. everything it's good and fine. I have a suggestion about your color variables naming. it's better to avoid specific color names (e.g --clr-cyan) as you used material design naming for other colors but this one it's not related to other ones. I recommend to read these two good articles about color naming:

    hope I could help you. good luck.☻

  • CyrusKabir•1,885
    @CyrusKabir

    Hi Matt, you did a good job on this challenge as a first real code without any tutorial. but there are some issues about your end result and code structure :

    • you used percentage value on border-radius property
    • using width instead of max-width on img
    • some some styles issues like color, padding, ... and now I gonna explain each issue in depth.

    1. what value it's good for border radius: at first step you should understand about differences between units in border radius I recommend read this stackoverflow post but in this simple challenge em and px are both fine units but in percentage value for having more closer result you need some calculations !

    2. more responsive images: using max-width property on images it's better because images won't take more width than maximum value in max-width property and using them with 100% value can make them more responsive. you can read more about responsive images on this article

    3. some little styling issue: your card heading doesn't have proper color like main design and having more space like padding between image and card inner would be great. you can use some comparison tools for matching your end result to main ui design like Perfect Pixel extension you can download it on their main site.

    I hope my little tips could help you

    Marked as helpful
  • CyrusKabir•1,885
    @CyrusKabir

    Hello 😃☺, you did good on this challenge and it's a very good way to learn new things you know learning new things or concepts in a language or a tool and implement them in real code or simple challenges like that. for your problem about changing background color in two states (mouse over and mouse click) as @Liam said you can use pseudo selectors in css like :hover, :focus, etc. and try to use some folder structure in your code base. I know this is a little project and you have only one .js file or .css file but adding those files to separate folders like any js code to js folder or script folder and any style or css file to css folder. this can help a little about code maintainability and other stuff. hope this could help you

View more comments