Skip to content
OverviewSolutions
11
Comments
139
CyrusKabir
@CyrusKabir

All 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

  • CyrusKabir•1,885
    @CyrusKabir

    Hello my dear friend ♥ you did good on this challenge and here some improvements for your component :

    • adding some transitions to your hover effects can make them smooth and better.
    • as you can see in hover state on your card img, your overlay opacity: 0.5 add some opacity to eye icon (svg) so in this design we can see the eye icon have not any opacity so here it's because of little difference between opacity and rgba or any color system with an alpha for opacity, and and you can read about this problem on this stackoverflow post difference between opacity and alpha.
  • CyrusKabir•1,885
    @CyrusKabir

    Hello my dear friend, you did a good job on this project and I liked your advice translating idea. and here some tips and improvements for your code and component :

    • add your advice to a <q> tag.the q tag use case it's for short quotation and browsers normally insert quotation marks around the quotation. you can read more about q tag in html HTML q tag
    • your dice icon for button it's not loaded because you should use ./ in url not / you can read more about this in this link
    • try to use addEventListener for more readable code and other things which you can find them in this stackoverflow post why are inline events bad
    Marked as helpful
  • CyrusKabir•1,885
    @CyrusKabir

    Hello, you did a clean and good work on this challenge but your landing page have a horizontal scroll on mobile and I test it with both dev tools and my mobile and still I don't know how to fix it

    Marked as helpful
  • CyrusKabir•1,885
    @CyrusKabir

    Hello my dear friend ♥ you did good on this challenge and here some tips for your code and result :

    • first, don't use id for css styles if you ask why? here is some good links and try to read them one after another : first: Specificity in css, second: reasons not to use IDs in css
    • actually, you don't need to change size of a pic or image with @media or any other ways, instead you can make an img tag completely responsive with just some few lines like this : img {max-width: 100%; display: block} with this you can have the guarantee the size of your pic or qr-img here for example never get more than it's container width
    • also you can check this good mini course in web.dev about responsive design in different concepts typo, images, ..... Learn Responsive Design
    Marked as helpful
  • CyrusKabir•1,885
    @CyrusKabir

    Hello my dear friend ♥ you did great on this challenge and only little problem is about accessibility issue with keyboard in Learn More button. I wasn't able to catch it with tab key or if I did it it wasn't noticeable

    • edit: can you add useful resource which you always use for clean code, work with animation, sass, css, .... ?
  • CyrusKabir•1,885
    @CyrusKabir

    Hello my dear friend ♥ actually the big reason it is you put your script tag before all DOM elements; here is some links about this problem : where-should-i-put-script-tags-in-html, script tag palcement, and the problem it's not just your tag placement after your js have whole DOM elements you add this style .active img .... for that arrow icon but you didn't add .active .answer {display: block} for showing answers. hope this solve your problem. good luck♥

  • CyrusKabir•1,885
    @CyrusKabir

    Hello my dear friend ♥ You did awesome job on this challenge and everything it's clean and readable and I was looking at your navigation bar in mobile and I noticed I can close it even with touching nav body it self so I looked at your isClickedOutside logic and the problem was there actually you don't have #nav element here because of it's child or nav__list position property so event.target.matches("#nav") it's always false then the second one event.target.closest("#hamburger") it's always true why ? because everywhere even nav body it self doesn't have parent with #hamburger id (sorry if my writing it's very bad :( ( ); but here you can change isClickedOutside body to this : event.target.matches('*:not(.nav__list)') && !event.target.closest('#hamburger') (I test it but you can test it too and have some discuss about it); edit : and yes of course if we click on the nav__link, again navigation get closed we can write anoter matches with && something like (event.target.matches("*:not(.nav__link)")) but it's a little messy and I am sure with right selector we can have cleaner code

    Marked as helpful
  • CyrusKabir•1,885
    @CyrusKabir

    Hello my dear friend ♥ you did very good job on this challenge and I just want say try using radial gradient for your card component, something like radial-gradient(circle at top,hsl(210, 19%, 18%) 0%,hsl(215, 23%, 14%) 50%, hsl(215, 27%, 12%) 100%)

    Marked as helpful
  • CyrusKabir•1,885
    @CyrusKabir

    Hello my dear friend ♥ you did very good on this challenge and I think it's perfect pixel no almost and UI it's good and I just want say some sassy things 😁

    • it's good to use @mixins when there is some arguments, I notice that you used slideInFromLeftanimation for just defining animation configs for different elements but you can use PlaceHolders in sass and just for changing animation delay you can add them in their classes, also you can check this article if you want 8 sass tips
    • you can name your media queries for more readability with sass, I mean using just some numbers with relative units it's not descriptive, you can check this articles for more information about naming @media in sass @media in sass, naming @media
    Marked as helpful
  • CyrusKabir•1,885
    @CyrusKabir

    Hello my dear friend ♥ you did good on this challenge and here some improvements :

    • for having circle shape on rating buttons your buttons should have equal width and height and also an 50% border-radius
    • try to change spacing values like padding or margin for getting close to main design as possible
    • you used disabled attribute to force user select a rating number but there is few problems :
    1. first, user can't submit buttons with keyboard (tab key), because it's disabled (accessibility issue)
    2. second, if user know how to inspect he/she can easily remove that disabled attribute on submit button and see thank you card without rating ! so good practice here is change validation check for that rating buttons and remember to print an error message when user did not click on a rating button and tried to hit the submit button
  • CyrusKabir•1,885
    @CyrusKabir

    Hello my dear friend ♥ everything is good but only problem here is accessibility issue with keyboard, I mean If I do not have a mouse, I can not work with your component

    Marked as helpful
  • CyrusKabir•1,885
    @CyrusKabir

    Hello my dear friend ♥ you did very good on this challenge and here some improvements for your component :

    • changing some typography here can make your component get closer to main design like adding line-height to body tag or higher value for card title font-size;
    • your component it's good for working with mouse :( but for example, if one day my mouse breaks down or many other things that force me to use the keyboard, there is no way to work with your component !!
    • and another important thing is, I can hit submit button without choosing any rating number !! (a little validation needed)
    Marked as helpful