Skip to content
OverviewSolutions
23
Comments
285
Agata Liberska
@AgataLiberska

All comments

  • Ricardo Fuentes Urbina•370
    @RicardoFuentes437Submitted over 2 years ago

    What do you guys think about the responsiveness? What do you think about the visual aspect of the page? does it look good? What are some practices that you think can be improved?

    Html, Sass, Javascript, Flexbox

    1
    Agata Liberska•3,995
    @AgataLiberskaPosted over 2 years ago

    Hi Ricardo, visually your solution is nice, doesn't overflow on smaller viewports which is probably the most likely issue on cards like this. However, you could definitely simplify your html here, at the moment pretty much every element like a heading or a paragraph is wrapped in a separate div and that's really not necessary. You're also missing the <main> landmark and the card could really be an <article>, that's the element that components like this tend to match the best.

    For your scores, you're using input type="button" which also isn't the best option. Buttons and button type inputs should be used to perform an action, for example favouriting a page, or adding a bookmark. Here, we're selecting one of 5 options - and if you have a number of options and have to only select one, this really calls for radio inputs.

    I would also treat this as a form and use a submit event to update the selected score.

    Hope this helps :)

    Marked as helpful
    0
  • Mario Lisbona•150
    @MarioLisbonaSubmitted over 2 years ago

    I found this project quite easy as i had just completed the 'intro component with sign-up form' with multiple input field validations.

    I wanted to improve it so i create a function with all the logic for the two different error messages for empty input or invalid input. This worked really well so i updated the previous project with a similar function which cleaned up the code.

    One issue i have is that i have used preventDefault() to that the form from being submited while the inputs are invalid. However, the form is still not submitting when all the inputs are valid

    Base Apparel coming soon page with CSS and JS for email validation

    1
    Agata Liberska•3,995
    @AgataLiberskaPosted over 2 years ago

    Hi Mario, page is looking really nice on desktop but something is off on mobile - on some screens the image looks to be covering up the heading. I think it would be a lot simpler if you didn't try to position it absolutely - have you tried just adding it with an img tag instead?

    The background image also is repeating which I'm not sure is the case in the original design.

    Nice work with form validation - I think you could maybe try to add validation as person starts correcting the email address? could be an extra challenge :) and as for preventDefault, it just prevents form from submitting to the page specified in the action attribute via the method specified in the method attribute. If you use this, you would need to provide an alternative way of handling the submission but don't worry about that for now. You can just set the input value to "" and display a success message to give it some pizzaz :)

    Marked as helpful
    0
  • Agata Liberska•3,995
    @AgataLiberskaPosted over 2 years ago

    Hi! Looking good and working just fine :) Definitely check out the accessibility report - the <main> landmark is missing, and the button we're using to update needs some type of labelling to be accessible. I'd also advise to add width and a height whenever you add an img - to avoid layout shifts.

    One probably very nitpicky thing - when I reload the card, the button and the quote marks keep moving up - this only lasts a fraction of a second but can look a bit like a glitch. I would suggest setting up a min-height for your advice-txt container so there always is some space there - even if it's just the height of a single line of text (I'd personally go for two lines and center vertically if it's just one line but that's your call). This layout shift wouldn't really be an issue because it happens after a user interaction but I think with some min height it would look more polished.

    Hope this helps :)

    Marked as helpful
    1
  • Agata Liberska•3,995
    @AgataLiberskaPosted over 2 years ago

    Hi! Nice work on this solution, visually I think this looks really nice. I would just work on the html - for a simple page like this, you're using quite a lot of nested divs which I personally think are unnecessary here, a lot of those could just be removed with no effect on the page. If you have a few block elements you don't really need to wrap them in a div unless you're grouping them for styling purposes etc. :)

    Nice work using a list for the social icons in the footer (those will need some labels/titles to validate though) but I think the three divs in 'we're different' section also should be an unordered list. And the mobile menu toggle should definitely be a button :)

    Hope this helps !

    Marked as helpful
    0
  • kefiiiiR•70
    @kefiiiiRSubmitted over 2 years ago

    Eventually, I suppose it works as it should work. I had a bit issue with active state of the buttons which I solved by using another loop inside the first loop (I could not think of another solution).

    Interactive rating, HTML/CSS/JS

    1
    Agata Liberska•3,995
    @AgataLiberskaPosted over 2 years ago

    Hello :)

    I think you made this a bit more difficult for yourself going for buttons with ratings where this really is a small form with 5 radio inputs - So I would visually hide the inputs themselves (key word is visually , display:none wouldn't be a great choice) and style the labels to be the circles you click and then pass this to the submit event :)

    And if you want to change the look of the buttons after pressing one, loop or forEach is an ok choice, but I would simplify your code by having an 'active' class and then just using classList.toggle('active')

    hope this helps :)

    Marked as helpful
    2
  • Shahid Khan•20
    @skafridi07Submitted over 2 years ago

    Although it seems easy, it took a lot of time and effort to specifically set the media query at max-width: 376px to ensure that the card layout behaves responsively. Because of the bem-css naming convention, which I used in this project, it is much simpler to read and debug the code. Despite the time commitment, it was worthwhile. The font-weights, font-families, and colors all have their own custom variables that I first set up. The layout was then constructed using Flexbox, and where necessary, margins and paddings were added. It was an excellent experience all around.

    Responsive card design using flexbox and media query

    1
    Agata Liberska•3,995
    @AgataLiberskaPosted over 2 years ago

    Hi Shahid! Card is looking nice :)

    One thing I would advise is to simply start with the mobile design instead of using max width media queries here. It's simply easier (mobile designs are usually simpler) and allows for fewer lines of css. For example on mobile, the image sits on top of the text container - which you achieve by setting flex-direction:column but if you started from mobile and added display flex at 400px and wider, this wouldn't have ben necessary at all because those elements just sit on top of each other by default.

    Another thing is that 378px is not really enough space for the card to display nicely, so I would move that query up to 400, maybe 450px?

    And lastly, for accessiblity, the image of the perfume definitely needs and alt text and you're also missing a <main> landmark :) and for more semantic html, <article> tag is perfect for cards like this

    Hope this helps :)

    0
  • Agata Liberska•3,995
    @AgataLiberskaPosted over 2 years ago

    Hi Nader, I think the background in this challenge is particularly tricky! I would have two images added in rather than just the one, set it to no repeat and then position each individually - or just add them in with the html, might be easier to position that way :)

    For the profile image, I would probably move it to your bottom div, so it's right at the top and then move it up using tranform:translateY property :)

    Other than that, I would have another look at what tags youre using here. <article> is a perfect tag to use for card components like this, and the stats should really be a list - and you definitely don't need to put each bit of text in it's separate div. I'd suggest something like this:

    <ul>
<li><span>80K</span> followers</li>

    And if I could give one more bit of advice, try to use class names that are a bit more descriptive. I think "profile" is good, "profile-card" would be better, and then instead of top/bottom something like cover or banner and profile-info? It will make your work a lot easier on bigger challenges! :)

    Hope this helps, happy coding!

    Marked as helpful
    0
  • Agata Liberska•3,995
    @AgataLiberskaPosted over 2 years ago

    Hi Eslam, I think it would be a good idea to review what html tags you're using here. It looks like you're using heading tags ust based on the styles you can see which isn't the best approach. Think about hierarchy here - it makes sense for the name of the perfume to be the main heading, but all other text could just be inside <p> tags or <span>s.

    I would also rethink your button. You've added hover styles to your green div which look nice but it would suggest to me that I can click this and something would happen - but only the text is an actual link. So Instead of using a div, you could just have an anchor tag and style it to look like a button:

    <a href="/"><img src="..."> Add to Cart</a>

    Make sure to take a look at the report above as well - the image urls should use / rather than \ and make sure to use landmark elements, <main> is definitely missing from here. I would also use <article> tag for the card.

    Also, for the perfume image I think adding a good alt text would also be very important, as this isn't a purely decorative image, it shows a product we're trying to sell.

    I also had a look at your css - I think in a simple project like this there is no need to combine your selectors, as you've only really got one thing with the class price or cart. Personally I think combining selectors really needs to be thought through as in a bigger project you may run into specificity conflicts and end up with a bunch of !importants which just isn't fun to work with :)

    Hope this help, happy coding!

    1
  • Odiesta•100
    @OdiestaSubmitted over 2 years ago

    I struggle to center the container. Then after a lot of googling i use display flex and align-items center. I was unsure whether to target the html tag in css or tag inside particular css class. Is it best to target html tag inside classes or id because it is more specific?

    QR Code Component with CSS

    2
    Agata Liberska•3,995
    @AgataLiberskaPosted over 2 years ago

    Hi! I think setting those flex styles on a container class is definitely a good idea. It makes it more reusable and if you were to add something else in the body, it wouldn't be affected. You could also add justify-content:center to your styles there to center horizontally - in your solution this is done with margin:auto which also works fine, but in a bigger project if you wanted to reuse this, I think I would prefer all centering styles to be under one class (rather than a container and the component itself if that makes sense).

    Another way to achieve the same result is to use display:grid;place-items:center; - just a matter of preference which one you go for.

    What I would like to see in your solution is some more semantic tags. so your container could be a <main> and then for the card itself, I would add an <article> tag inside the container. Also don't forget to add a width and height to your image :)

    If we're also looking at accessibility, it might be a good idea to add a link around the QR image :)

    Marked as helpful
    3
  • imad•3,330
    @imadvvSubmitted over 2 years ago

    The checkbox was a letter bit tricky for me to understand at first, but I was able to found some blog post in css-Tricks that help me out, please feel free to give your feedback 😊

    Pricing component with toggle solution

    1
    Agata Liberska•3,995
    @AgataLiberskaPosted over 2 years ago

    Hi Imad!

    So there needs to be some improvements here when it comes to matching the design - you have a min height set on the cards, which stretches them unnecessarily. In general with components like this there is no need to set the height like this, instead I would control the size of the element with the content and some padding. Other than that I think it looks nice :)

    With the checkbox input though - first of all it really does need to either have a label or at least an aria-label. Also, I think it's working the other way round? The price for monthly shows up as 249, and annually as 24.99 :D no worries, it's an easy mistake to make when you're focusing on how to build something you've never done before!

    Anyway, I'm not sure that a checkbox here is the best option - I know that it is probably the most common design pattern with toggles like this so I don't blame you for choosing it! But you're really choosing between two options here (instead of choosing to have something included or not) - which really calls for radio inputs to be used. A checkbox would be for something optional which you can choose or ignore, which isn't the case here. I've got one or two projects which have a toggle built on two or three radio buttons if you want to take a look, but I am also certain that Grace Snow has done this exact challenge in a beautiful way (I know because I totally used it as a cheat sheet when building my solutions :D) oh here it is

    Anyway hope you find this helpful, happy coding! :D

    Marked as helpful
    1
  • Chris Ekstedt•110
    @cekstedtSubmitted over 2 years ago

    Are there any best practices, in semantics or styling, that you would suggest as an improvement to my code?

    QR Code Component

    2
    Agata Liberska•3,995
    @AgataLiberskaPosted over 2 years ago

    Hi Chris! Nice work, your solution looks just like the design, and it's good to see some semantic elements :) One advice I would give is that it's a good idea to get in the habit of including width and height on your img elements to avoid any shifts in layout - that's something that google would pick up on in audits and score you down for. I would also link to google fonts in your <head> element in your stylesheet rather than importing it from css - that way it loads at the same time as the stylesheet - right now it will only download after the browser is done downloading and parsing your stylesheet. Doesn't make much difference when files are tiny but when your code base grows, it can have an effect on loading :)

    Marked as helpful
    0
  • Agata Liberska•3,995
    @AgataLiberskaPosted almost 3 years ago

    Hi Lucas, nice work on styling here - your solution looks great. There are some overflow issues on smaller devices which you may want to look into though, and I'd definitely modify your media query - you've set the width of the card to 800px, but making the switch from mobile to desktop at 700px - so not quite enough space for the card.

    I would also recommend to look at accessibility here - a person using keyboard to navigate your solution wouldn't be able to toggle any of the accordions - the toggle should definitely be a focusable element, like a button.

    I would also like to be able to open more than one accordion at a time :)

    Hope this helps!

    Marked as helpful
    1