As always, I welcome any comments and suggestions you may have.
Mohamed ELIDRISSI
@elidrissidevAll comments
- @imadvvSubmitted over 2 years ago@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 totype="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 likerem
s orem
s.
Hope this helps you, if you have some questions feel free to reply. Good luck!
Marked as helpful1 -
- @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?
@elidrissidevPosted over 2 years agoHey 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 anh1
and the rest including the button is in theApp
component. What I would do in this case is extract the whole card to theAdvice
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 helpful1 - @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.
@elidrissidevPosted over 2 years agoHey Sara π,
Congrats on finishing your first challenge!
To address your questions:
- 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.- 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 adist
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 - There is a workaround to the focus issue, and it's the
- @Mahmoud-Elshaer-10Submitted about 3 years ago
Your opinion matters to me. Please provide your feedback so I can improve. Many Thanks.
@elidrissidevPosted about 3 years agoHello 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 usingarticle
tag for the reviews, this element requires a heading inside, so you can make all the review titles anh2
. 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 helpful1 - @PresidentTreeSubmitted about 3 years ago
I do not think anything is wrong with the code, but any feedback is still appreciated!
@elidrissidevPosted about 3 years agoHey 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 byh2
. If you wanna take it one step further, you can include anh1
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 addoverflow: 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 helpful0 - Every page can have at max one
- @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!
@elidrissidevPosted about 3 years agoHey 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
andbutton
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 theimg
. It looked a bit confusing at first, so one way I'd do it is to inline thesvg
in your HTML. That way you can setfill="currentColor"
and they'll inherit the parent element'scolor
value so you can change it with ease.
That was all I have, I hope it was not overwhelming. Good luck!
0 - Always make sure your
- @flp-pcllSubmitted about 3 years ago
Hello, everyone!
This is my solution to the FAQ accordion card challenge. What do you think?
Thanks!
@elidrissidevPosted about 3 years agoHey Felipe, great work on this solution!
Here are my suggestions:
- Just in case you don't know, you can build the whole accordion without a single line of JavaScript using
<\details\>
and\<summary\>
elements. - The way you've built the accordion works perfectly. Here's a good read about creating accessible accordions
Good luck, and keep it up!
Marked as helpful0 - Just in case you don't know, you can build the whole accordion without a single line of JavaScript using
- @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!
@elidrissidevPosted about 3 years agoAmazing 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
div
s where appropriate, for example the "Cancel" text can be inside aa
or abutton
element because it conveys that it can interacted with. - Check the accessibility report page for more.
Good luck to you!
Marked as helpful1 - @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!!
@elidrissidevPosted about 3 years agoGreat 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 intorem
s andem
s. - 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 - Avoid using pixels, especially for things like
- @nomatter-pySubmitted about 3 years ago
I'm only learning, need your suggestion and pieces of advice
@elidrissidevPosted about 3 years agoGreat 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 helpful1 - The "Why Us" section is a list of reasons to choose this fictional service. So it makes sense to use a more semantic element,
- @brkclnSubmitted about 3 years ago
thanks for the feedback ^!
@elidrissidevPosted about 3 years agoHey 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 helpful2 - In the design, the "Claim Your Free Trial" button has a shadow from the inside. You can achieve this in CSS by using
- @Frontend-WizardSubmitted over 3 years ago
If you can change 2 things, which they will be?
@elidrissidevPosted over 3 years agoHey! Great job with this one! I actually have 5 things π :
- Avoid skipping heading levels, you went from
h1
toh4
. 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 afooter
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 helpful0 - Avoid skipping heading levels, you went from