Skip to content
OverviewSolutions
16
Comments
16
Mohamed ELIDRISSI
@elidrissidev

All comments

  • Mohamed ELIDRISSI•455
    @elidrissidevPosted over 2 years ago

    Hey Imad 👋

    Hope you're doing well. You did a great job on your solution! I usually see some common mistakes in most solutions, but not this one!

    Here are my notes:

    • type="submit" is meant to be used with buttons inside a <form>, in this case there is no form so I'd suggest you set it to type="button".

    • The "Why Us" section is meant to be a list of features of this fictional website, therefore I think a <ul> will be more semantically correct instead of a <p>.

    • Some page elements are styled with their class names whereas others are styled by their tag name, this makes the CSS difficult to read as you'll need the HTML as a reference. Consider leaning more towards classes to style your elements, as they are re-usable and they make your code easier to understand.

    • The ch unit has some specific use cases, but generally I'd stay away from if not needed and just use other units like rems or ems.

    Hope this helps you, if you have some questions feel free to reply. Good luck!

    Marked as helpful
    1
  • Naveen Ongole•80
    @Naveen39OSubmitted over 2 years ago

    This is the first time I have used an API request in the challenge. I have used React to build the app and I used useState hook to store the data. Is this ok? or Should I call the API request from useEffect hook?

    Responsive advice generator app

    1
    Mohamed ELIDRISSI•455
    @elidrissidevPosted over 2 years ago

    Hey Naveen 👋,

    Great work on your solution, keep on going!

    I have some feedback for you, first is an accessibility one:

    • The dice button doesn't have any descriptive text inside. For accessibility reasons, buttons and links must always have some text inside that describes their action, in the case where it's not a part of the design, consider making the text visually hidden.
    • The dice image is mainly for decoration, and when you add a proper title ☝️, there will be no need for accessibility software to announce the alt text, in this case, you can make it empty so that it's not announced.

    This one is coding-style related:

    • Try to keep things consistent in your code so other people find it easier to read. For example, you have some random spaces in your JSX between the attribute and value. In general, try to follow the common conventions, in this case, no spaces between attribute and its value in JSX.

    This one is related to React:

    • Know when and what to separate something to its own component. For example, you have created an Advice component, but it only contains an h1 and the rest including the button is in the App component. What I would do in this case is extract the whole card to the Advice component.
    • I would also recommend separating data fetching logic to a custom hook. For example, you can create a hook called useAdvice and inside of it you would put the state and the fetching function, you would then return those to be consumed by the component. e.g:
    function useAdvice() {
  const [advice, setAdvice] = useState(null);
  const [isLoading, setIsLoading] = useState(false);
  const [error, setError] = useState(null);

  const fetchAdvice = () => {
    setIsLoading(true);
    axios.get("https://api.adviceslip.com/advice")
      .then((res) => 
        {
          setAdvice(res.data.slip); 
          setIsLoading(false);
          setError(null);
        })
      .catch((error) => 
        {
          console.log(error);
          setError(error);
          setIsLoading(false);
        });
  }

  return { advice, isLoading, error, fetchAdvice }
}
    • I would remove the hard-coded advice that's shown initially on page load, instead you can replace it by a useEffect with an empty dependency array that will fire when the component mounts and fetch the initial advice from the API.

    Phew, that was a lot 😅. Hope it helps, Good luck!

    Marked as helpful
    1
  • Sara•70
    @skuenziSubmitted over 2 years ago

    When using :active :hover and :focus states, what's your method for maintaining accessibility while keeping the design clean? Whenever :focus is activated, it remains until you click outside the element. How do you prevent that?

    What is your method for file organization? I'm used to having a src folder, but the index has to be in the root for most hosting sites.

    Advice Generator Using Vanilla JS

    1
    Mohamed ELIDRISSI•455
    @elidrissidevPosted over 2 years ago

    Hey Sara 👋,

    Congrats on finishing your first challenge!

    To address your questions:

    1. There is a workaround to the focus issue, and it's the :focus-visible pseudo-class, quote from MDN docs: The :focus-visible pseudo-class applies while an element matches the :focus pseudo-class and the UA (User Agent) determines via heuristics that the focus should be made evident on the element. (Many browsers show a "focus ring" by default in this case.)

    But keep in mind that this is not supported in Safari, so you'd want to have a fallback style using the regular :focus as mentioned in the link.

    1. For code organizations, I usually use a bundler such as Parcel, that way I can organize my files under src folder and also get to use modern javascript features. When ready to deploy, Parcel compiles everything together under a dist folder by default which you can configure to be the root folder used when deploying to a service (I know vercel and netlify have it, not very sure about github pages).

    I hope this answers your questions, feel free to ask if you have more and good luck!

    0
  • Mahmoud•185
    @Mahmoud-Elshaer-10Submitted about 3 years ago

    Your opinion matters to me. Please provide your feedback so I can improve. Many Thanks.

    HTML CSS

    1
    Mohamed ELIDRISSI•455
    @elidrissidevPosted about 3 years ago

    Hello Mahmoud 👋 You've done a great job with your solution, It looks good on all screen sizes and design comparison shows only very little differences!

    Here are some things to improve:

    • Consider taking a look at the Report page of your solution when you submit it, it will help you identify problems and fix them.
    • Your page lacks a level-one heading (h1). It is very important for SEO and even accessibility reasons to have one in every page of any site. In cases where the heading in not part of the design (like here), you can include it but make it visually hidden. Additionally, because you're using article tag for the reviews, this element requires a heading inside, so you can make all the review titles an h2. Remember to always follow heading levels on your page from 1 to 6, don't skip them!
    • The text of the reviews is quoted from their authors. So wrapping the text of the reviews inside a blockquote element is more correct semantically.
    • This one might be a bit more of a personal preference, but I'd lean to using classes more and avoid selecting elements directly in CSS, this makes styles easier to read and more re-usable.

    This is all the feedback I have, I hope it was helpful. Good luck!

    Marked as helpful
    1
  • Mohamed ELIDRISSI•455
    @elidrissidevPosted about 3 years ago

    Hey there! Great job with this one, you've made it look very close to the design. I noticed some points that you can improve in your solution:

    • Every page can have at max one h1, you have used 3. Consider replacing them by h2. If you wanna take it one step further, you can include an h1 but make it visually hidden since it's not in the design to make it only readable by screen readers.
    • You've made the heading of every card all uppercase. While that's how it looks in the design, making them all uppercase in your html means screen readers will interpret them as an abbreviation (i.e. they will spell it word-by-word). To fix this, you should write them in normal casing in the html and add a text-transform: uppercase to the headings.
    • This one is a minor detail: You can avoid having to set border-radius on the first and last card by setting it on their parent. After doing this you'll see the corners still sharp so to fix it you need to add overflow: hidden to clip the edges of the cards sticking out.

    This is all I have, I hope it was helpful. Good luck to you!

    Marked as helpful
    0
  • Frances•165
    @frances-mSubmitted about 3 years ago

    I'm curious about how web developers handle development for different devices - in my case, when using dev tools to view this site on an iPhone X screen everything looks good. But on my own iPhone X, the social media buttons are warped. I can troubleshoot that issue because I own the device, but are there other tools to help a developer be confident that their layouts will render properly on each specific device?

    Thanks for taking the time to check out my project!

    Form Validation with Vanilla JavaScript & Sass

    1
    Mohamed ELIDRISSI•455
    @elidrissidevPosted about 3 years ago

    Hey Frances! Hope you're doing great. First, I just wanna say congrats because you did an amazing job at making this as pixel-perfect as it can be! Unfortunately, regarding your question I cannot provide much help, personally I don't remember I had an issue like that but I could be wrong. Though I have some suggestions regarding your solution:

    • Always make sure your a and button elements have text inside them that describes where that link takes to or what that button does. In your solution for example, the social share buttons should probably be links btw because they will redirect somewhere, but should also have text inside them rather than just images. In this case the design does not show the text, so you can use the "visually hidden technique" which hides the text visually while still making it readable by screen readers.
    • For the social icons, I saw you're using filter to change the color of the img. It looked a bit confusing at first, so one way I'd do it is to inline the svg in your HTML. That way you can set fill="currentColor" and they'll inherit the parent element's color value so you can change it with ease.

    That was all I have, I hope it was not overwhelming. Good luck!

    0
  • Mohamed ELIDRISSI•455
    @elidrissidevPosted about 3 years ago

    Hey Felipe, great work on this solution!

    Here are my suggestions:

    Good luck, and keep it up!

    Marked as helpful
    0
  • Aliaa•25
    @aig0041Submitted about 3 years ago

    Hey everyone! I just completed this project and I know my code is really sloppy and I'm sure it could have been written in a more organized way, I just believe that the best way for me to learn is to dive in and apply anything that I know, so that's what I did.

    This is the first time that I use flexbox in a project and I'm still not really sure I understand it fully, so I'd really appreciate if you point out any mistakes I have with it specifically.

    Again, I'm only a beginner and I know there is so much I don't know yet, so any suggestions or tips to improve the code overall would be really appreciated. Thanks!

    Order Summary Component with HTML, CSS, & Flexbox

    2
    Mohamed ELIDRISSI•455
    @elidrissidevPosted about 3 years ago

    Amazing work Aliaa! Not bad for a first challenge. Keep it up!

    For the images problem, it's because of the URL you're using:

    • http://127.0.0.1:5500 is a URL only accessible on your machine when you were running the http server (Using Live Server Vscode extension?).
    • file:///Users/... is a URL used to access files on your local filesystem (Mac in this case) and is again specific to your system and won't work elsewhere.

    So the fix for this is to use a relative URL, in your case it's gonna be ./images/<image name>.

    As for the rest I only have 3 suggestions:

    • Every html page should have a level 1 heading h1 that describes the main content of the page.
    • Consider using semantic html elements instead of generic divs where appropriate, for example the "Cancel" text can be inside a a or a button element because it conveys that it can interacted with.
    • Check the accessibility report page for more.

    Good luck to you!

    Marked as helpful
    1
  • Jaonary Andritiana•70
    @tix04Submitted about 3 years ago

    Just starting out on frontendmentor and this is my first challenge. Can anyone rate my project from 1 -10 and send me any feedback on any errors you find or any improvements I can make. Whether it is on my naming conventions, How clean my code is, or just improvements to help me become a better developer. Thank you for your time!!

    Responsive component

    1
    Mohamed ELIDRISSI•455
    @elidrissidevPosted about 3 years ago

    Great start Jaonary! Definitely not bad for a first challenge. Keep it up! Here are some suggestions for you:

    • Avoid using pixels, especially for things like font-size, margin, padding, width, height etc. Reason being is that they don't scale with your browser's font size setting. To verify this, try increasing your browser's font size from 16px to something like 20px and do a comparison between your solution and this site. See how in your solution nothing changes, while in frontendmentor the page sort of zooms in a bit? There will be special occasions where this won't matter, but as a general advice I will suggest that you look into rems and ems.
    • Decorative images that don't add anything to the page content can have an empty alt text as per the W3C Web Accessibility Initiative:

    Decorative images don’t add information to the content of a page...In these cases, a null (empty) alt text should be provided (alt="") so that they can be ignored by assistive technologies, such as screen readers.

    • A very small detail but I'd add a cursor: pointer to the "Proceed to payment" button to give the feedback that it's clickable.

    That's all I have. Good luck to you!

    1
  • Mohamed ELIDRISSI•455
    @elidrissidevPosted about 3 years ago

    Great work overall, like @hardy mentioned just try to use less intensive shadows by increasing alpha value in shadow color and increasing the blur. Additionally I have a couple of suggestions:

    • The "Why Us" section is a list of reasons to choose this fictional service. So it makes sense to use a more semantic element, ul. It looks ugly by default but you can always style it according to the design.
    • In your CSS, I see you're using a mix of a responsive unit (rem) and absolute unit (px) which is something you'd want to avoid because absolute units do not scale with the browser's font size setting, you can learn more about responsive units in this article.

    Good luck, and keep up the good work!

    Marked as helpful
    1
  • Mohamed ELIDRISSI•455
    @elidrissidevPosted about 3 years ago

    Hey Burak! Great work on this challenge, keep going! Here are some advises on things to improve:

    • In the design, the "Claim Your Free Trial" button has a shadow from the inside. You can achieve this in CSS by using inset: box-shadow: inset 0 -0.25em 0 rgba(0, 0, 0, 0.1)
    • For the "Claim Your Free Trial" button, you've made the text all uppercase in your HTML. This means that screen reader software will spell it out letter-by-letter as if it was an acronym. In this case, it's better to style the button with text-transform: uppercase to make it uppercase visually.
    • Form validation works, but only shows one error at a time. I have a solution for this challenge where I'm using JavaScript's Constraint Validation API which I think will help you a lot, feel free to check it out.
    • Additionally you can do some tweaking on the font sizes and spacing to make it look closer to the design.

    I hope this didn't overwhelm you. Good luck!

    Marked as helpful
    2
  • Mohamed ELIDRISSI•455
    @elidrissidevPosted about 3 years ago

    Hey! Great job with this one! I actually have 5 things 😅:

    • Avoid skipping heading levels, you went from h1 to h4. You should always go from 1 to 6. You can always style them differently if they don't match your design.
    • I would wrap <div class="attribution"> inside a footer element since it's more meaningful.
    • For the illustration image, since it's purely for the look, I would keep alt empty to make it clear for assistive technology that it's a decorative image so they won't read the alt text.
    • Consider using classes to style your HTML elements instead of directly referencing them, it's reusable, more readable and overall a better practice for bigger projects.
    • I would stay away from absolute units like pixels because they're not responsive and don't scale when browser's font size increases/decreases.

    I hope this didn't overwhelm you. Good luck!

    Marked as helpful
    0