Skip to content
OverviewSolutions
11
Comments
27
Devmor
@devmor-j

All comments

  • Devmor•470
    @devmor-jPosted over 2 years ago

    Hey this is a great solution 🎉

    One quick tip:

    I think the hero text would be easier to read if you increase line-height a bit. I tried line-height: 1.4; and it already looks good.

    Hope this helps and cheers 😀

    Marked as helpful
    0
  • Devmor•470
    @devmor-jPosted over 2 years ago

    Great this is almost identical well done 🎉

    One important tip:

    Right now this app might not fetch new advice on Firefox browsers because they rely on cache and we have to enforce the browser to call API service on each user request.

    This can be easily done by adding {cache: 'no-store'} option to our fetch service like so:

    inside ==> advice-generator/src/services/api.js

    const callToApi = () => {
  return fetch('https://api.adviceslip.com/advice', { cache: 'no-store' })
    .then((response) => response.json())
     .........

    Hope this is helpful and have a nice time coding 🤩

    Marked as helpful
    2
  • Devmor•470
    @devmor-jPosted over 2 years ago

    Hey this is great ✨

    You did it, it looks good. Here are some of my suggestions and answer to your question:

    1️⃣ Even though the sizes are fine on 100% zoom (initial zoom on most browsers), the ux is not very well when user zooms in/out. I went through your code and I suggest get rid of absolute position on .cont class and overall it's not a best practice to use it anyway (unless we have to). In this case we can achive centering simply with these refactoring:

    First remove these rules from .cont class:

    .cont {
    top: 50%;
    left: 50%;
    width: 75%;
    height: 60%;
    overflow: hidden;
    position: absolute;
    transform: translate(-50%, -50%);
}

    Then insert these properties to .cont class:

    .cont {
  /* adjust 56rem size (this is just a good starting point) */
  width: min(100%, 56rem);
  margin: auto;
}

    Finally add these rules to your body element to center it's content:

    body {
  display: flex;
  min-height: 100vh;
  margin: 0;
}

    Now you have a great zooming capability and did not mess with the positioning (which means easier styles to maintain).

    2️⃣ Your image overlay looks ok but if you want to be percise then try to use mix-blend-mode: multiply then adjust opacity around 0.8.

    3️⃣when you've applied these refactorings, don't forget about @media queries and make sure their fine too.

    I liked your solution and keep going ✌ 🦄

    Marked as helpful
    1
  • Mohamed Metwally•50
    @mmetwally0Submitted over 2 years ago

    Would definitely love to hear feedback about the button's shadow & ways to improve it, and the design overall. Thanks in advance!

    Order summary component

    3
    Devmor•470
    @devmor-jPosted over 2 years ago

    Hey this is nice 🎉

    One small suggestion I can make is to remove height: 90vh from card-container class and instead add padding-bottom: 2rem.

    Everything else is working as expected, keep going and have fun 😃 🌹

    0
  • Devmor•470
    @devmor-jPosted over 2 years ago

    Well done🏆

    One thing I like to suggest is to shrink the hero image a bit. To do so I tried this inline css and it worked:

    /* on second .hero__item where the image resides */
.hero__image {
  width: min(100%, 24rem);
}

    Also decreasing padding on .section__hero would help too. In my opinion hero sections should not have a scrollbar (of course on a real website this is not a concern but here it will look much better).

    Overall I enjoyed your well written SCSS and you did great, keep rocking ✨🌹

    Marked as helpful
    0
  • Devmor•470
    @devmor-jPosted over 2 years ago

    Great 🏆🎇 The functionality is there and it works. But there are some minor styling issues 🎨. If you like CSS refactoring (which I highly recommend), These are my suggestions:

    1️⃣ main content should fill the whole screen space (because it has a gradient):

    /* add these properties to these rules */
main {
    ...
    min-height: 100vh;
    align-content: center;
    padding: 2rem;
    ...
}

.main {
    ...
    /* 50rem is the total width of the faq card, adjust if not happy with it */
    width: min(100%, 50rem);
    ...
}

    📝 remove these extra height properties:

    html {
    height: 100%;
}

body {
    height: 100%;
}

    2️⃣ in mobile view there is not enough space to show decorative image and content side by side. so either remove it for screens below '40rem' or stack them on top of each other (I personally have removed it):

    @media (max-width: 40rem) {
    .image {
        display: none;
    }

    .faqs {
        width: 100%;
    }
}

    3️⃣ class of .faqs has a complex padding and specially on mobile view causes a big empty gap next to arrows, so maybe '2rem' or '3rem' should suffice:

    .faqs {
    ...
    /* adjust to your own preference */
    padding: 2rem; 
    ...
}

    4️⃣ your image trick is awesome and I liked it 🥰

    5️⃣ class of .arrow could improve a lot, here is my version:

    .arrow {
    transition: transform 0.3s ease-in-out;
    align-self: center;
    object-fit: contain;
    width: 0.75rem;
    margin-left: 0.75rem;
    user-select: none;
    pointer-events: none;
}

    ❗ and to update your accessibility report (saying about 'role=main', you have to manually generate a new one (there is a red button to do so next yo your report). Same thing goes for screenshots.

    Have a nice time and cheers 😄🎉

    Marked as helpful
    0
  • Devmor•470
    @devmor-jPosted over 2 years ago

    Hey great job 🎉

    I've been crawling through your code (cloned your repo 😀) and here are some tips if you're interested in other's opinions:

    • 1️⃣ Mobile view is very small because you missed the most important css3 viewport meta tag. Just add this tag to your index.html and everything will fix:
    <meta name="viewport" content="width=device-width, initial-scale=1.0">

    This will bring in some issues with sizing and I'm sure you can easily deal with it but feel free to ask any questions if stuck.

    • 2️⃣ I suggest remove any extra .container from your style that just makes it complicated and hard to debug and this simple app does not need that high specificity caused by so many class names.
    • 3️⃣ Try not to set width sizes using un-responsive units like 'px' and instead you can use 'rem' as a good starting point. I usually set my cards width anywhere from '16rem' to '24rem'.

    And that's it for now.

    This is important to know that you did the job done and this is good.

    So don't make it too hard on yourself and just keep going; One day you will find yourself highly confident in your code 💪😎👊

    Marked as helpful
    1
  • Devmor•470
    @devmor-jPosted over 2 years ago

    Nice! this is good and the job is done.

    If you're interested in other's opinion then I can make 2 suggestions:

    1️⃣ Maintaining two separate css files for Mobile and Desktop is a thing of past and is not recommended since Responsive design has become the standard in Web dev today. So eigher merge and keep all CSS in one file or at least make sure styles look great on Mobile design and only after that justify your styles to look nice on Desktop as well.

    2️⃣ Zoom out and make sure media queries don't break on smaller viewport. Right now this website does not look styled on my system (1080p screen).

    I think this media query is the reason:

    @media (min-width: 376px) and (max-width:1440px) {
    ...
}

    Since your applying these rules only between 376px - 1440px so If someone load your site on a 4k monitor, these media query styles are not applied.

    Your HTML though looks nice and solid, well done and hope these help 😀

    Marked as helpful
    1
  • Devmor•470
    @devmor-jPosted over 2 years ago

    Hey, this looks almost identical, great job 😎.

    To clear that accessibility issue on report just wrap your whole content in a main tag.

    If you prefer a more semantic html, one suggestion I can provide:

    It's a best practice to put multiple heads [h1s, h2s, ...] inside a hgroup tag.

    that being said:

    <hgroup>
      <h1 class="line-one">
        Reliable, efficient delivery</h1>
      <h2 class="line-two">
        // Also this span seems superfluous
        // if `span` is used to indicate bold text, 
        // then `<b>...</b>` tag reads better in my opinion (or `strong` tag)
        <span>Powered by Technology</span>
      </h2>
</hgroup>

    If you don't like hgroup simply do like so:

    <h1 class="line-one">
        Reliable, efficient delivery
      <b class="line-two">
        Powered by Technology
      </b>
</h1>

    I would personally go with the latter because I like having only one h1 on each page (article, post, ...). This will bring slightly better SEO results and helps robots to find content easier.

    Hope these help, just for you to know you're doing great keep it up, cheers 😀

    Marked as helpful
    0
  • Alice Won•20
    @alicewonSubmitted over 2 years ago

    My updated code has cleaner usage of the advice state and includes a setTimeout to handle the 2 second caching from the API. Thanks for the comments!

    There was something I couldn't quite figure out with my state , the dice button doesn't seem to update if you click on it too fast consecutively... almost like you have to wait 1 second before being able to see a new string. I tried to use prevState to prevent batching re-renders but I don't think that helped. I would love if this was smoothly/quickly updating the advice data each time I clicked the button.

    The glowing hover state looks a little odd to me so any tips on how to make it more nuanced/pretty would be great!

    I improvised a bit of a title page/homepage because when my default state (an empty advice object) would render on refresh, I'd have just quotation marks and "advice #..." without any data yet. I ended up tinkering with conditional rendering and template literals to achieve somewhat of a homepage based on if the dice button had fetched the data for my object in state yet (for example, was advice undefined? if so, render "Welcome" string", etc). Could I have done this another way that's maybe more efficient?

    It was also my first time using Vercel and it was super easy to set up and get going.

    Advice Generator using API and React

    3
    Devmor•470
    @devmor-jPosted over 2 years ago

    Hey, this is good.

    About your question, that delay is by design as the API noted:

    Note: Advice is cached for 2 seconds. Any repeat-request within 2 seconds will return the same piece of advice.

    So you should disable button for 2 sec after each request sent, this way user knows that they have to wait a moment.

    Use setTimeout that would help.

    Marked as helpful
    0
  • Saleh•390
    @SalehAbuhusseinSubmitted over 2 years ago
    1. What I found difficult is Classes Naming When I do them on my own it becomes a great mess and when I do BEM Naming I find nesting the blocks and stuff confusing If there is a guide for naming that would be awesome.
    2. Is There advantage to create tags and the text with JavaScript.

    HTML SCSS Vanilla JS

    1
    Devmor•470
    @devmor-jPosted over 2 years ago

    Hey 🙋‍♂️ great job on completing this challenge.

    In my opinion no matter how good you are at naming CSS classes, it usually ends up in a mess!

    If you are familiar enough with CSS and have good styling skills, maybe it's time to use a framework like TailwindCSS. At least worths a try.

    This framework changed the way I write CSS and I am still happy with it.

    For example instead of using display: flex on tons of classes and ship a bunch of classes that share properties, Tailwind just inserts one class named flex on your element and that class is used on any other element with a flex class on it.

    This code below is a div element with a .container class on it:

    <div class="container">
    ... // children elements
</div>

>> inside style.css
.container {
    display: flex;
    ... // other properties
}

    Tailwind version of that is:

    <div class="flex ...{other classes}">
    ... // children elements
</div>

    And in build process Tailwind will add those classes in a css file:

    >> inside style.css
.flex {
    display: flex;
}
... // other classes (almost atomic classes - meaning only one property at a time is declared)

    Now you don't even name a class just use what you need.

    Please note there are other similar frameworks like WindiCSS and UnoCSS as well but currently TailwindCSS is the standard utility-first CSS framework in terms of adoption (personal opinion).

    Hope this helps and cheers 😄

    0
  • Devmor•470
    @devmor-jPosted over 2 years ago

    You did a great job on this challenge 😍 and everything is working as expected.

    There is only one tiny suggestion I can make just as the report says:

    Simply put all main content into one main tag so search engines and screen readers could have a better understanding of your HTML structure.

    Top level div with class of dashboard can be converted to a main tag but check and make sure nothing breaks:

    <main class="dashboard">
    ...
</main>

    Hopefully that should be it. Keep it up 💪

    0