@emanuelcba94
Submitted
Any suggestions on how I can improve are welcome! Thank you!
@asbhogal
@emanuelcba94
Submitted
Any suggestions on how I can improve are welcome! Thank you!
@asbhogal
Posted
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:
font-size
in rem
for accessibility purposes. This article by freeCodeCamp explains it in detail Link330px
, the parent container is touching the browser boundary. You could add some padding or margin just to give it some white/breathing spaceHope this helps!
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.
@asbhogal
Posted
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.)
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
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
height
with a min-height: 100vh
to prevent any vertical overflowHope this helps!
Marked as helpful
@andrijaivkovic
Submitted
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!
@asbhogal
Posted
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:
_Country.scss
file is empty. Presumably your styles are referenced elsewhere, in which case this can be removedHope this helps!
Marked as helpful
@VickyAzola
Submitted
Hi 👋
Please try my solution, it should show an error if you try to submit the form empty or if your email is invalid. I also added an alert box in case the email is valid.
Any feedback is welcomed, thanks 🤞
@asbhogal
Posted
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:
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. Linknpm
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
1000px
and 768px
, it might be worth reviewing and changing the breakpoint classes for when the content flows vertically<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 LinkHope this helps!
Marked as helpful
@fotouh
Submitted
This challenge was so easy and nice to Do and website design and color were amazing and excited 🤩 Bs Fatoh Mohamed 01111389125
@asbhogal
Posted
Hi Fatoh,
This is a good solution - well done! I have noticed a few things however I wanted to highlight below when refactoring:
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 widthsmax-width
value (in rem
) and a width: 100%
on your parent container and let the elements in them occupy the space they need naturally<header>
outside of your <main>
as this is semantically incorrectfont-size
should always be in rem
for accessibility reasons. Here's a good article by freeCodeCamp explaining this in further detail Linkabsolute
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<a>
tags as they link to an external site, preferably also within individual <li>
elements as part of an unordered list (<ul>
)<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 LinkHope this helps!
Marked as helpful
@ANDyouNo
Submitted
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
@asbhogal
Posted
Hi there,
Good work! The designs match the mockup nicely. Just a few things I thought worth mentioning:
inp
and errMsg
as well as typos (succesCard
) as it can become difficult for developers to debug and identify their purpose especially as code scalesrem
values for accessibility purposes. Here's a good link from freeCodeCamp explaining this in further detail Linkwoff2
format then upload and reference them in your stylesheets. This video by Kevin Powell explains this process in further detail Linkwidth
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
valuevariables.scss
, a responsive.scss
, a typography.scss
etc.)Hope this helps!
Marked as helpful
@NawalMalki
Submitted
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!
@asbhogal
Posted
Hi Nawal,
This is a good solution, well done! Just noted a few things below:
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.
Icon
, use hamburgerToggle
and instead of Show
use mobileNav
)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"); });
woff2
format as this is designed for the web. Here's a link from W3 explaining in further detail LinkHope this helps!
Marked as helpful
@igux1
Submitted
It was my first project alone, I'm studying for around a week, all feedbacks on how it could've been better, and other tips will be very apreciatted.
@asbhogal
Posted
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
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.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 providedcontainer
in a main
element for accessibility purposes - it helps screen readers and assistive technologies identify where the primary content of the document body
is locatedrem
for accessibility purposes. Here's a good link explaining this in further detail LinkHope this helps!
@oscarmumm
Submitted
My solution for Order summary card chalenge
@asbhogal
Posted
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:
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 Linksection
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)main
container touches the viewport boundary. It would be worth adding a padding
or margin
to the sides just to give them some spacingHope this helps!
@JooseMM
Submitted
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/
@asbhogal
Posted
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:
Hope this helps!
Marked as helpful
Hi! 👋, Frontend Friends.
I've just completed my first front-end coding challenge. This is my solution for the QR Code Component.
🛠️ Built With:
📦Features:
🔖What I am learned:
❓Questions for the community:
💡Any suggestions on how I can improve are welcome!
😊I'll be happy to hear any feedback and advice! Thank you.
@asbhogal
Posted
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
@zZedx
Submitted
idk why but some images are not showing up in live website
@asbhogal
Posted
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:
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
@CodingEule
Submitted
Hello,
not happy at all with the desktop version. Thankfull for any clue, how to improve.
Thanks
@asbhogal
Posted
Hi Pascal,
This is a good attempt. I've outlined below what you need to do to get it closer to the mockups:
width: 100vw
and width: 300px
on your parent container. Neither should be used, as they cause overflow issues, which is what's happening here. You should set a max-width
of, for e.g., 1000px
and a width: 100%
so anything below the max-width
in the viewport will mean the content will occupy the full space it can.img
elements are being used in the div
with a <picture>
element for them instead. Here's a link from MDN explaining this Link600px
.Hope this helps!
Marked as helpful
@jr19981010
Submitted
I'd love to see your constructive feedback upon my project
@asbhogal
Posted
Hi there,
Good work! I can see you've made a solid attempt at matching to the mockups. I've noticed several things however which are worth considering for refactoring this:
font-size
from a value set at a high viewport width down to the the lowest set viewport width in rem
(note that this assumes a base font-size
of 16px
which is suitable generally, but it's worth noting in larger applications this would require further adjustments.) Here's a link to generate the function Link<header>
should always be at the top, outside any other element (except the body
) and your main content should be wrapped in the <main>
element to mark where the most interactive content is (important for accessibility.) You could remove the two <div>
elements and style the first two I mentioned..woff2
format as this is designed for the web. Here's a link explaining this in further detail Link<img>
elements are missing alt
attributes, which are important for screen readers and other assistive technologies to be able to read them to establish their purpose and contextvar
when declaring variables - I noticed this in your var classname = span.getAttribute('class');
. This should be const
seeing as it isn't being reassigned within the same scopeHope this helps!
Marked as helpful
@rodrigompires
Submitted
Made with React, and some CSS animations with theme changes. Click on my avatar to open a modal.
@asbhogal
Posted
Hi Rodrigo,
This is a really interesting approach and it's great to see some out-of-the-box approaches, with rem units for font-size
also used.
I'd say in terms of typography, keep these consistent as the Elegant version has multiple versions across different cards. The font is also hard to make out on some of these cards across the themes because the background/foreground contrast ratio is very low, making it difficult in particular for screen readers and those with assistive technologies to read.
Also, if you're using React I'd suggest deploying to a more robust platform like Vercel or Netlify which handles caching, performance and stability much better. They have CI/CD and debugging to identify errors during build and if you use Vite (more on this below) it will automatically build your application with each Git push, without needing to run an npm script.
Following on from this, avoid using CRA as Meta have stopped supporting this. For client-side applications like this, use Vite, which supports vanilla, React, Angular etc.
Locally host your Google Fonts for privacy and performance reasons. Since you're using an npm environment, you can install the Noto Sans typeface using Fontsource, import the weights you need and declare the properties in your elements. Here's a link to documentation Link
With your directories, I'd suggest splitting your styles from your logic files by using separate folders (ie. a css
folder with your component stylesheets, since you already have separation of concerns)
Hope this helps!
@webdevhill
Submitted
Hello frontendmentor.io coders,
The provided background .svg for desktop is only 1440px. On xl screens, it obviously does not span the device's screen. There are resources for expanding the SVG view box but that seems beyond the parameters of the style guide. Any opinions on style guide parameters and expectations or solutions for extra large screens?
Thank you
@asbhogal
Posted
Hi Jeffrey,
This is great work - well done!
To handle the background images they should ideally be placed in a <picture>
element in your markup, with paths to the mobile and desktop versions which change respective to a specified viewport width. I've added a link here from MDN detailing this and how to implement it Link
Also, locally host your Google Fonts for privacy and performance reasons. Here's a good video explaining why and how to achieve this Link
Hope this helps!
@AslamtoIbrahim
Submitted
I don't know how to add suitable size to my elements adding width height min-height max-width I want to know more about px rem vw?
@asbhogal
Posted
Hi Aslam,
When you use sizing, you should use rem
units. There are converters which use the base of 16px
to convert these for you. In general with containers, you should use a max-width
on the parent container with an explicit value (ie. unit) and a width: 100%
with @media
queries for padding with smaller devices. For eg., if your max-width
is 350px
, by the time you get down to that value in the viewport, the content will touch the sides, so you can add a padding
property for the body
element in a @media
query for this width so you still have some spacing. The max-width
means that with viewports above that width, it will always stay at that fixed value, and with viewports below it, the content will take up the full (ie. 100%
) width of the parent container.
You should also avoid setting explicit heights
to the parent container and width
and height
properties to the child elements inside it because this can cause overflow issues. It's best to let them occupy the space they need to naturally.
With this, hopefully you can refactor your code with these changes. If you need any further help, let me know.
Marked as helpful
@ibrahimherith
Submitted
Hi everyone,
This is my solution to the calculator app challenge. I learned alot of things while doing it. Hope you enjoy it.
Quick question though, how do you guys include one javascript file into another file? for example you have first.js and second.js files and you want to include both in main.js file, whats the trick?
@asbhogal
Posted
Hi Ibrahim,
Great work! In answer to your question, you export your functions and then import these as modules into another file (whether that's a second, or a main one.) This takes advantage of ES6, and conventionally you use a bundler to handle this, as vanilla environments can't handle module compilation without it. The easiest one to use in this case is Vite, which supports vanilla (pure HTML, CSS and JS) files, React, Angular etc. If you deploy these to Vercel, the platform will re-build your project with each Git commit. If you're new to these practices, I'd be happy to send you some links to documentation etc.
Hope this helps!
Marked as helpful
@lorenacrincon
Submitted
Hi everyone!
I would appreciate it if you could give me some feedback on the structure of my code to see what I can improve.
Thank you!
@asbhogal
Posted
Hi Lorena,
This is a very good solution, well done! The designs match the mockups nicely and it's great to see a React approach to get some early exposure to state management.
Just a few pointers below worth considering when you refactor and for future builds:
darkMode ? body.classList.add("dark") : body.classList.remove("dark");
here, if darkMode is true (by default by itself means it evaluates if it is truthful first), then parse the instruction after the ?
, or if it is falsy, parse the instruction after :
npm
environment, you can use Fontsource to install the typeface, then import the specific font-weights you require and then declare the typography via font-family
, font-weight
etc. in your stylesheets. Because you're using Tailwind, you can refer this in the theme
or extend
object in your tailwind.config.js
file Here's a documentation link for the Inter font Link and how to customize the Tailwind theme LinkHope this helps!
Marked as helpful
@rdonatiello1
Submitted
Hi Everyone -
Thank you for checking out my solution and (hopefully) providing some valuable feedback. This is my first project so I really appreciate any helpful tips you can provide. The biggest challenge for me was centering the container that holds all of the elements. I ended up figuring it out but I feel like there's probably a much more efficient way to accomplish it. Let me know what you think.
Cheers.
@asbhogal
Posted
Hi there,
This is a good attempt, and I can see why you've used certain code. I've made several suggestions below when you refactor this:
absolute
to position this card and instead use flex
properties (ie. display: flex
; flex-direction: column
) and margin: 0 auto
to centralise itbody
parent element of the card by also using flex
properties and height: 100vh
to vertically center the card (height: 100vh display: flex; flex-direction: column; justify-content: center;
)max-width: 320px
, for e.g., on the parent container, and a width: 100%
, which means any value above 320px
will keep it fixed at that, and any width below will take up the full length (remember to convert these px
to rem
values)max-width
property on the child img
element and replace this with width: 100%
. You also won't need the margin
property..img-block
element as this doesn't serve a purpose here (presumably this was to be the parent container which wraps the img
? Either way you can remove this)rem
) units for positioning elements, instead of %
because these are more variable and prone to cross-browser responsiveness issuesHope this helps!
Marked as helpful
@semperprimum
Submitted
I would greatly appreciate your feedback!
This was a tough one..
It was my first time building a carousel, and I used Swiper JS for it. The experience was quite challenging. I encountered numerous issues, such as flickering, and the carousel occasionally skipping some items. However, after investing several hours into it, I managed to fix the problems, and now it seems to be working properly.
The grid part, on the other hand, wasn't as difficult as i thought it would be. It was my first time incorporating grid-areas
in the project, and I found them incredibly helpful. Using media queries with them was a breeze!
@asbhogal
Posted
Hi Bogdan,
This is a great solution - well done! You've used grid well here, as documented, as well as rem values and a reset, and its responsive. Nice. Just a few things I thought I'd mention which is more from a management/maintainability perspective:
Skills.jsx
, to avoid repeated code, you can declare an array of objects with key-value pairs for the icon, text and classname (instead of using them as props) then map() over this array and render a <Card />
component for each, passing in the array index for the key prop and the respective three properties.App.scss
into individual components for easier management and debugging - it reduces cognitive load, as isolating a single element from a long stylesheet can take some time. You can search for it, but I t think its better to approach component styling the way you approach component logic and split them.Also, from a personal perspective, I think using either styled components or SASS in a project is best, instead of combining them. While there isn't a hard rule for this and some would argue against it, having one approach (either coupled for single management or separated concerns) means it'll be easier to locate and debug styling and possible prevent code clashes. That's just my two cents.
Hope this helps!
Marked as helpful
I wanted to make it responsive, but I don't know how
@asbhogal
Posted
Hi Jhonata,
To make this responsive, you can either use flex-direction: column
in a @media
query or flex-wrap
, or grid
, which would retain the structure. The choice of when to use either depends on the content and how you want it to adapt, but here's a good link explaining the difference between the two. Link
Before you do this however, I'd suggest making several changes to your code because these are causing issues to your layout:
margin-left
on the section
as this is causing issues with viewport width changes. Presumably this was added to try and centralise the container? In any case, use justify-content: center
and remove the width: 50%
on the .results .sumario
.width
on the parent .conteudo
container as explicit values shouldn't be set on this. Set a max-width
instead, with a width: 100%
, so anything below the max-width
value will occupy the full width of the viewport and above, it will stay at the fixed max-width
.height
to 100vh
to keep it vertically centered as, same as above, you shouldn't be using explicit values for these, as they constrain content and cause overflowing issues (ie. when content inside spills outside because the container isn't adapting as it has a fixed value set.)Hope this helps!
@ofthewildfire
Submitted
Single Price Component Grid Solution
Following the road-maps on Discord I have completed this challenge and my main focus was accessibility and the responsive aspect of the challenge. I am still learning accessibility and the topic is pretty dense, however, I did as much as I could on my knowledge, namely:
Layout not breaking or looking too weird when user changes font size on their browser/ and or zooms in. I tested this with Chrome, font-size at Very Large and zoomed in to 200% (as per W3.org links I read) and content shifted to maintain its form.
Font-size changing with view-port, I used clamp() for this, if there is a better way, please let me know.
I changed the colours, they were failing colour contrast checks, so I did my best to pick colours that still looked decent and matched. They now pass those checks.
Things I Need to Fix
Unsure about
Since I am still learning and things may be totally whacky any and all advice is greatly appreciated - thanks, and have a good day.
@asbhogal
Posted
Hey Kaycee - this is very good, well done!
I really like the changes you've made to the palette and considerations to accessibility which often get overlooked at times unfortunately in front-end dev. You've documented your process nicely and your code is well organised. Props to you.
In terms of keyboard accessibility, the main thing is to ensure the button
is focusable when visitors use the tab
on the website to navigate through. In order to do this, it's best to remove the transparent
value on the outline
as this removes the focus on the element so they don't know if it's been selected. Here's a good link which might help illustrate this Link
With regards to the heading alignment issue, I suspect this is due to the space-around
value assigned to both containers, as this calculates gaps both in between the child components and outside. It does this based on the height of the container against the total height of the elements and equally distributes the difference. The spacing that the flex
has created on the outside of the right is greater than the left, thereby causing the headings and list section to be 'pushed in' and subsequently lower than the heading on the left, if that makes sense. I've added a link here explaining this in further detail Link In this case, you could try removing the justify-content
altogether (which will default it to the space-between
from the main>.grid_item
declaration, pushing them to the edges of the top and bottom) and adding a specific padding to these two, e.g. padding: 1em 1.5em
which will set an equal amount and thereby keep the headings the same height from the top.
You could also change the background-color
to #205857
to flow nicely with the right container and change the colors of the h1
to #205857
and the h2
to #197371
. Visually I think this blends better and keeps the color palette to a simple accent of two tones and white.
I've ran your site through an online accessibility checker for WCAG 2.0 (Level AA) and Level AAA and found no potential or known issues - great news. Here's the link to the checker which you can also use Link
Hope this helps, and again, really great job. Impressive work!
Marked as helpful
@moonrose93
Submitted
any feedback?
@asbhogal
Posted
Hi Isidora,
Great work! The accordion seems to function nicely. I've noted some things down below worth considering for refactoring:
481px
and 640px
you have some overflow issues and spacing caused by the explicit height
value set on the parent container. Ideally you should avoid this, as it constrains the child elements. It's best to let them occupy the space they need naturally.grid
for this to keep the intrinsic values of the left and right columns consistent as the viewport changes. This ensures the content isn't compressed. Here's a good link explaining the difference between the grid
and flex
and when to use one instead of the other Linkif
statements with a ternary operator, for e.g.
addSupp.addEventListener("click", () => { paraFive.style.display = (paraFive.style.display === "none") ? "block" : "none"; hideParaTwo(); hideParaThree(); hideParaOne(); hideParaFour(); })
This says that if paraFive.style.display is equal to "none", it sets paraFive.style.display to "block", otherwise, it sets it to "none", and is great for single-line parsing for when the condition is evaluated (ie. if it is truthy, it returns the "block" for the display
and if its falsy, it returns "none")Hope this helps!
Marked as helpful