Skip to content
OverviewSolutions
6
Comments
62
Aman Singh Bhogal
@asbhogal

All comments

  • Aman Singh Bhogal•1,010
    @asbhogalPosted over 1 year ago

    Hi Emanuel,

    Great work! The design matches the mockups well, your logic is clean and it's great to see grid used for this. Just a few pointers I wanted to mention:

    • Always have your font-size in rem for accessibility purposes. This article by freeCodeCamp explains it in detail Link
    • Locally host your Google Fonts for privacy and performance reasons. Here's a good practical video showing how to do this Link
    • At viewport widths <= 330px, the parent container is touching the browser boundary. You could add some padding or margin just to give it some white/breathing space

    Hope this helps!

    1
  • Ghadah AlMuhaideb•10
    @GMuhaidebSubmitted over 1 year ago

    I found it difficult to push the code to github using VS code terminal I continuously faced "error: failed to push some refs to 'https://github.com/GMuhaideb/qr-code-component.git'". However, I upload it traditional way using drag and drop the files.

    Mobile Code First QR Component

    1
    Aman Singh Bhogal•1,010
    @asbhogalPosted over 1 year ago

    Hi Ghadah,

    This is great work, well done! I'd definitely look into the Git terminal issues as soon as you can to avoid any problems with workflows in future projects (unless this is a localized incident.)

    • You have a typo in your text - "bulding"
    • You're missing an alt attribute value, which is important for screen readers and other assistive technologies to be able to understand the purpose and context of the img
    • Your container should ideally be placed inside a <main> element for accessibility purposes as well - it helps direct users to the area of the most prominent content of the document body
    • Always locally host your Google Fonts for privacy and performance reasons. Here's a good video by Kevin Powell which goes through the process of doing this Link
    • You can replace the height with a min-height: 100vh to prevent any vertical overflow

    Hope this helps!

    Marked as helpful
    0
  • Andrija•560
    @andrijaivkovicSubmitted over 1 year ago

    Hello! This is my solution to this challenge. Everything was implemented as shown in the challenge as well as the optional theme switcher.
    Hope you will find my solution useful and if you have any suggestions or criticism feel free to write about them!

    Rest Countries API App

    1
    Aman Singh Bhogal•1,010
    @asbhogalPosted over 1 year ago

    Hi Andrija,

    This is brilliant work - well done! The design matches the mockups really well and its great to see context used for global native state management. Just a few things I've noticed, more from a back-end and maintenance perspective:

    • Avoid using CRA for future projects, as Meta no longer supports this. For client-side applications such as these, Vite is your next best bet, it's relatively low-config and compiles SASS out-of-the-box (you'll just need to install it as a dev dependency as normal)
    • Always locally host your Google Fonts. You can install the typeface you need via the Fontsource dependency, then import the font weights required and declare the relevant properties needed. Here's a link to documentation for the font used here Link
    • Your _Country.scss file is empty. Presumably your styles are referenced elsewhere, in which case this can be removed
    • Netlify throws a 404 error on a page refresh/re-directs (this is a very common problem with React routing, I had the same issue with Vercel.) I've added some documentation here which will hopefully resolve this on your end Link

    Hope this helps!

    Marked as helpful
    1
  • Aman Singh Bhogal•1,010
    @asbhogalPosted over 1 year ago

    Hi Victoria,

    This is great work, well done! The design matches the mockups nicely and the validation seems to function well too. Just a few things I've noticed which are worth considering when refactoring:

    • It's best to use npm packages for your Tailwind instead of a CDN, as this isn't optimized for production (the CDN is designed for in-browser development using online sandboxing tools.) Using the npm version means your build is optimized, compressed and minified and you can also customize your configuration (more on this below) Here's a link to some documentation on how to get this up and running Link I recommend using Vite to handle the compiling of the Tailwind utility classes into standard css. If you're unfamiliar with this, I've also added a link here on how to handle this. Link
    • You should locally host your Google Fonts. With npm environments, you can install the typeface through the Fontsource dependency, then import the weights you need and reference them in your tailwind.config.js file (either by extending or overriding the theme.) I've added some links below regarding this
    • Your image is compressed between 1000px and 768px, it might be worth reviewing and changing the breakpoint classes for when the content flows vertically
    • I can see you've referenced both the images for mobile and desktop, however I can't confirm whether they change respective to the viewpoint. You should ideally have these within a <picture> tag which achieves this by specifying the width for the mobile version, for e.g., to display. Here's a link to an MDN article explaining this in further detail Link

    Hope this helps!

    Marked as helpful
    0
  • Aman Singh Bhogal•1,010
    @asbhogalPosted over 1 year ago

    Hi Fatoh,

    This is a good solution - well done! I have noticed a few things however I wanted to highlight below when refactoring:

    • The page isn't fully responsive unfortunately - between 980px and 401px you have compression of the image and overflow of the right-hand side content. I'd suggest reviewing this using your dev tools responsive device viewer and changing the direction/layout for these widths
    • Following on from this, avoid setting fixed width values to child or parent elements (either explicit, or as %) as these cause layout and spacing issues and force content to adapt in not ideal situations. You should have a fixed max-width value (in rem) and a width: 100% on your parent container and let the elements in them occupy the space they need naturally
    • You also need to take your <header> outside of your <main> as this is semantically incorrect
    • Your font-size should always be in rem for accessibility reasons. Here's a good article by freeCodeCamp explaining this in further detail Link
    • I wouldn't say it's necessary to have an absolute positioning for the social icons. Since this is in a footer, you could use either flex or grid to align these to the right and they will respond much better as the viewport width changes
    • The icons, used here as Font Awesome classes, should be inside <a> tags as they link to an external site, preferably also within individual <li> elements as part of an unordered list (<ul>)
    • The images should ideally be managed in the <picture> element in your markup instead of using CSS, with paths to the separate files for desktop and mobile and a viewport width of when the respective ones should appear. Here's a link from MDN explaining this in further detail Link
    • Always locally host your Google Fonts for privacy and performance reasons. Here's a good video showing how to do this Link
    • Also, I couldn't see your stylesheet in your GitHub code (I could only see this through the browser console) - may just want to check this on your end so other developers can look through it easier

    Hope this helps!

    Marked as helpful
    1
  • Andrey•180
    @ANDyouNoSubmitted over 1 year ago

    Hello everyone, Here is YouNo. It's my first splash into SCSS. Please feel free to share any suggestions or comments you may have. <3

    Responsive sign up form

    2
    Aman Singh Bhogal•1,010
    @asbhogalPosted over 1 year ago

    Hi there,

    Good work! The designs match the mockup nicely. Just a few things I thought worth mentioning:

    • You might want to review your JS code. When I enter in incomplete, invalid or correct email addresses, the page re-renders, so as a user it's unclear which are submitted (only the last should be) and the error messages don't show.
    • Also, try and keep your variables descriptive enough to understand - avoid condensing them to inp and errMsg as well as typos (succesCard) as it can become difficult for developers to debug and identify their purpose especially as code scales
    • It's best to have your font-size as rem values for accessibility purposes. Here's a good link from freeCodeCamp explaining this in further detail Link
    • Locally host your Google Fonts for privacy and performance. You can download them from the website, convert them to woff2 format then upload and reference them in your stylesheets. This video by Kevin Powell explains this process in further detail Link
    • Avoid setting an explicit width value to parent containers as these can cause overflow issues with viewport width changes. It's best to have a max-width with a specified value and a width: 100% so the content either occupies the full available space on viewports less than the max-width or the maximum it can based on the max-width value
    • From a maintenance perspective, split your stylesheet into separate partials for easier location and reduced cognitive load (e.g. having a variables.scss, a responsive.scss, a typography.scss etc.)

    Hope this helps!

    Marked as helpful
    1
  • Nawal Malki•160
    @NawalMalkiSubmitted over 1 year ago

    Hi there 👋, I’m Nawal and this is my solution for this challenge.

    Note that it's only responsive at (max-width: 375px)

    Any suggestions on how I can improve and reduce unnecessary code are welcome!

    HTML , CSS and JS page with media queries

    1
    Aman Singh Bhogal•1,010
    @asbhogalPosted over 1 year ago

    Hi Nawal,

    This is a good solution, well done! Just noted a few things below:

    • Avoid using var when declaring variables. It's best to use let for those which are redeclared later, or const for those that aren't. In this case, you can use const.
    • Variable names should also be in camel case (meaning, the first letter is always lowercase and the first letter of the next word in it is uppercase, e.g. first and firstNumber) and here they should be more clearer (to help other developers identify exactly their purpose, which becomes important especially as your code scales, e.g. instead of Icon, use hamburgerToggle and instead of Show use mobileNav)
    • It's better to let CSS handle the visibility and styling of elements rather than manipulate them in JS (for performance reasons). You can refactor the code to introduce a specific class to display the element as a block and the use the toggle() function in JS to show this via .classList.toggle() on the icon event listener (ie. hamburgerToggle.addEventListener("click", function () { mobileNav.classList.toggle("show"); });
    • It's great you've locally hosted your font, however this should be in woff2 format as this is designed for the web. Here's a link from W3 explaining in further detail Link

    Hope this helps!

    Marked as helpful
    0
  • Aman Singh Bhogal•1,010
    @asbhogalPosted over 1 year ago

    Hi there,

    This is a really good first solution! The design matches the mockup nicely - well done! It's also great to see you've locally hosted your fonts too (and in the correct format.) I've listed a few points below worth considering when refactoring this

    • To make this responsive, you can use either flex or grid. I can see you've used flex on the child elements, so it's best in this case to also add this property onto the parent container and adding a flex-direction: column on a width of, for e.g. 640px (using @media queries.) By default, the flex-direction is row, so this doesn't need to be explicitly set.
    • It's also best to avoid setting explicit height and width properties on your parent containers, as this can cause overflow issues as the viewport width changes. Instead, use an explicit max-width value, and a width: 100%, so at any viewport width below the max-width, the content takes up the full amount and at any viewport width above the max-width, the content remains fixed within it. Flex will then take care of adjusting the elements within the space provided
    • For semantic purposes, wrap your parent container in a main element for accessibility purposes - it helps screen readers and assistive technologies identify where the primary content of the document body is located
    • Make sure to have your font-size in rem for accessibility purposes. Here's a good link explaining this in further detail Link

    Hope this helps!

    1
  • Aman Singh Bhogal•1,010
    @asbhogalPosted over 1 year ago

    Hi Oscar,

    This is great work, well done! The design matches the mockup well. Just a few things I've noticed which are worth considering for refactoring:

    • It's best to use the picture element to handle responsive images, instead of the CSS background-image property. Here's a good article from MDN explaining how to do this Link
    • You can simplify your code by removing the section container, applying these properties to the main and just having the two divs inside this instead (since it's good practice to have both the max-width and width properties on a parent container)
    • Always locally host your Google Fonts for privacy and performance reasons. Here's a good video by Kevin Powell which shows how to do this Link
    • At 320px your main container touches the viewport boundary. It would be worth adding a padding or margin to the sides just to give them some spacing

    Hope this helps!

    0
  • Jose•80
    @JooseMMSubmitted over 1 year ago

    Hi! The only issue for me was that in Firefox the fetch method was pulling data out of the cache memory instead of the URL provided as an argument. I fixed it specifying the cache policy as it's mention in this article.

    https://hacks.mozilla.org/2016/03/referrer-and-cache-control-apis-for-fetch/

    Advice Generator responsive app

    1
    Aman Singh Bhogal•1,010
    @asbhogalPosted over 1 year ago

    Hi Jose,

    Great work! The design matches the mockup nicely and the app is responsive and functions well (I like your coding practices here too with the custom hook and useEffect implementation. Nice.) Just a few things which are worth considering:

    • Always locally host your Google Fonts for privacy and performance reasons. Since you're in an npm environment, you can install the typeface as a Fontsource dependency, import the weights require then declare the relevant properties in your styling. Here's the documentation for the Manrope font Link
    • Avoid using Create React App, as Meta no longer supports this. For client-side applications like these, switch to Vite.

    Hope this helps!

    Marked as helpful
    1
  • Kanishka Priyashantha•690
    @kanishkasubashSubmitted over 1 year ago

    Hi! 👋, Frontend Friends.

    I've just completed my first front-end coding challenge. This is my solution for the QR Code Component.

    🛠️ Built With:

    • Semantic HTML5 markup
    • SASS/SCSS
    • Node.JS
    • GULP.JS
    • Mobile First Approach

    📦Features:

    • Responsiveness (Mobile/Tab/Laptop/Desktop).

    🔖What I am learned:

    • Level Up SCSS
    • CSS Flexbox & Grid. Link

    ❓Questions for the community:

    • I have a question about best practices. Is the Flexbox best layout module to design flexible responsive layout structure?

    💡Any suggestions on how I can improve are welcome!

    😊I'll be happy to hear any feedback and advice! Thank you.

    Responsive QR Code Component Solution Using CSS Flexbox Layout Module

    1
    Aman Singh Bhogal•1,010
    @asbhogalPosted over 1 year ago

    Hi Kanishka,

    Great work! In answer to your question, flexbox is generally the best modern layout method to use for flexible responsive structures. Grid would be used for content where you want better control of their layout using columns and rows. This article explains it quite well Link as well as this video by Kevin Powell which demonstrates in practice when you would use which Link (NB. You can also combine them in an application, it all depends on the use-case)

    Also I've had a look at your code and semantically you could change this to just have the child elements as div and img inside the main tag, you don't necessarily need the article as this serves a different purpose Link

    Another thing - make sure to always locally host your fonts for privacy and performance reasons. Here's another good video which shows how to practically do this Link You can also download your typeface using Fontsource npm packages, then import the weights you require and reference them in your elements. Super easy and convenient Link

    Hope this helps!

    Marked as helpful
    0
  • Aman Singh Bhogal•1,010
    @asbhogalPosted over 1 year ago

    Hi Kartik,

    The reason why most of your pictures are showing is because of the / at the beginning of the src path. Remove these where they're present and your images will show.

    Just a few other things:

    • Locally host your Google Fonts for privacy and performance reasons. Here's a good link explaining why and how to do this Link
    • Use clearer variable and class names, ie. "notification" instead of "noti", as this can make it harder for other developers to read (always code with a mindset of others reading it to work on or debug it)
    • Make sure to add an alt attribute for screen readers and other assistive technologies (this helps them to understand the purpose and context of images for accessibility)

    Hope this helps!

    Marked as helpful
    0