Skip to content
OverviewSolutions
11
Comments
140
CyrusKabir
@CyrusKabir

All comments

  • Elaine•11,400
    @elaineleungSubmitted over 2 years ago

    I took on the extra bonus challenge of wiring everything up without Javascript, and this needed some creative ways to solve the problem of keeping the inputs as siblings to the plan/card elements so that the price information can be changed. This means I can't have the radio inputs nested at all in any containers. I also didn't want to resort to using a checkbox after reading Sara Soueidan's post on building accessible/inclusive toggle switches. Lastly, I wanted to make sure there's some transition between the values so that it's obvious to the user that something is different; even though I could reuse my code from the previous challenge, this also took some time to figure out. I think everything looks and works fine, and hopefully the semantic HTML won't cause issues, but I prefer using Javascript in the future.

    Responsive toggle component with only CSS and HTML

    3
    CyrusKabir•1,885
    @CyrusKabirPosted over 2 years ago

    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.

    1
  • CyrusKabir•1,885
    @CyrusKabirPosted over 2 years ago

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

    Marked as helpful
    0
  • dannxvc•180
    @dannxvcSubmitted over 2 years ago

    Hi! I just developed this interactive rating component with vanilla JavaScript, and I learnt a lot about accessibility, I read some articles related to this so I decided to use radio buttons for the options and I used css without losing accessibility. This article helped me through this, and I hope it can help more people as well.

    I appreciate any feedback about my code. Thank you.

    An interactive rating component developed with vanilla JavaScript

    2
    CyrusKabir•1,885
    @CyrusKabirPosted over 2 years ago

    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
    1
  • CyrusKabir•1,885
    @CyrusKabirPosted over 2 years ago

    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.☻

    2
  • Matt Wagner•10
    @MattLearnstoCodeSubmitted over 2 years ago

    This is my first attempt at any kind of off-tutorial project, and I'm pretty happy with the outcome.

    Honestly the hardest part was getting it deployed to my page. As far as the layout and design goes it looks right on my browser but I bet there are a dozen little things I could do to make it cleaner/better/more responsive, so any feedback is greatly appreciated.

    QR Code - HTML and CSS

    1
    CyrusKabir•1,885
    @CyrusKabirPosted over 2 years ago

    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
    1
  • Keagan Dickinson•160
    @PhisherFTWSubmitted over 2 years ago

    I found having the hover effect while being able to click the button to change the background color quite difficult and still don't have it working right, I tried doing it with the onmouseover and onmouseleave. Still, then I couldn't get the button to stay clicked when I went to click the submit button, I know my code is very rough, I have only recently learnt DOM javascript and this was my first time putting it into a project, any. All feedback is absolutely welcomed, and offering alternate and better solutions, please feel free.

    Thanks!

    interactive-rating-component

    2
    CyrusKabir•1,885
    @CyrusKabirPosted over 2 years ago

    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

    1
  • Amir Hashemi•480
    @AmirhosseinHashemiSubmitted over 2 years ago

    I hope you review my code and help me to fix my problem and write cleaner code.

    Html & Css - Flexbox

    3
    CyrusKabir•1,885
    @CyrusKabirPosted over 2 years ago

    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.
    0
  • CyrusKabir•1,885
    @CyrusKabirPosted over 2 years ago

    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
    1
  • Karol Binkowski•1,640
    @GrzywNSubmitted over 2 years ago

    Hey!

    I'm really happy with the result :)

    I turned my spaghetti JavaScript code into cleaner and more maintainable OOP import/export module based code. Animations are made using GSAP and ScrollTrigger plugin. If you know SOLID principles or some design patterns and you think I could have done something better - let me know, since it's my first OOP JS project after all. I'll probably code the next one in TypeScript.

    The solution isn't pixel perfect, but it's not that bad. I spent some time working on it, but I thought it wasn't worth it, especially improving "partners" section was hard. (I didn't want to make a modifier class for every image and then compare it to the design, since this section is broken in Figma).

    Feel free to test out things and check out my code. I might have missed something, like I always do. Any helpful feedback is appreciated!

    Mobile-first solution with SCSS && GSAP / ScrollTrigger animations

    3
    CyrusKabir•1,885
    @CyrusKabirPosted over 2 years ago

    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
    1
  • Jack Shelton•210
    @thejacksheltonSubmitted over 2 years ago

    I tried using a media query, and found changing the size of the QR code based on the screen size a little troublesome. Is there a better way to go at this? Maybe using ems and rems?

    QR Code Component

    2
    CyrusKabir•1,885
    @CyrusKabirPosted over 2 years ago

    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
    0
  • CyrusKabir•1,885
    @CyrusKabirPosted over 2 years ago

    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, .... ?
    1
  • Saman•120
    @samanthewebdevSubmitted over 2 years ago

    Overall this project was easy but its not working js in it isn't working and i check the code many times but idk why still not working!

    faq-accordoin-card-component

    1
    CyrusKabir•1,885
    @CyrusKabirPosted over 2 years ago

    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♥

    1