Any feedback on how to improve my coding style would be highly appreciated. I didn't use Sass for this solution though.
Simeon Udoh
@simeon4realAll comments
- @simeon4realSubmitted almost 5 years ago@simeon4realPosted almost 5 years ago
Thanks a lot @mattstuddert . I appreciate your feedback on my solution. I've learn a thing or two from your response. I do appreciate. Thank you
0 - @pytengSubmitted almost 5 years ago
Is it better to use a helper class or select all elements that you're going to style the same?
@simeon4realPosted almost 5 years agoCongrats on your submission @Teng. Regarding your question, I usually use helper/utility classes as it helps on even big projects as well as small ones. I noticed you used BEM. That's sweet! Keep it up.
You can take the step further and fix the outlined accessibility issues generated by the accessibility report. Once again, congratulations!
If you found my recommendation useful, I'll appreciate if you upvote my feedback. Thanks
2 - @zac-heiseySubmitted almost 5 years ago
This was tough layout for me, especially with getting the size and positioning right for the SVGs (particularly under the 'Features' section). Any insights or advice on working with and properly sizing SVGs is much appreciated!
I also used <details> and <summary> for the disclosure portion of the page under 'Frequently Asked Questions.' I'm curious how others approached this, and if using
<details>
and<summary>
presents any usability/accessibility issues that I'm not seeing.Thanks in advance for having a look!
@simeon4realPosted almost 5 years agoHello Zac, I must say, I am ver impressed with your solution. This is stuff way above my skill level and I liked it. I'm yet to attempt this challenge myself. Aside from the accessibility issues pointed out in the accessibility report, I have just one suggestion to add.
Rather than resizing each heading and text font sizes individually, I think you should add
html { font-size: 62.5% }
This way, 1rem will always be === 10px. Calculation: 100% รท 16px(browser default) * 10px === 62.5%. Now, your font-size is 10px aka 1rem in your root.
What you could do on smaller screen sizes is to change the size of your root to say, 8px.
html { font-size: 50%; }
Now, 1rem === 8px. Calculation: 100% รท 16px(browser default) * 8 === 50%.
If you found my recommendation useful, you can upvote my feedback. I would appreciate that.
2 - @VincenzoMarcovecchioSubmitted almost 5 years ago
I went back to this challenge and i completed in the best way possible with my knowledge level at the moment, i believe i can do better what do you think?
@simeon4realPosted almost 5 years agoHello Vincenzo, Congratulations on your new solution on this challenge. Like you, I plan on going back to all the projects I worked on to apply my new Javascript knowledge to them. I'm still learning like you. ๐ About your submission, I noticed some things which could be improved.
- First, Start using relative units instead of fixed units like pixels. Plus we don't know which devices our users will access our web apps from. Also, CSS is supposed to flow like water and adapt to any screen size you throw at it. But you can't do that if you defined fixed values with Pixels.
So I suggest you start using relative units like rem and em. Trust me, they will make your life easier as they will scale to new screen sizes..
-
Your HTML isn't that Semantic. You used way too many divs though it doesn't matter much since this is a relatively small project but you should look it up.
-
Move that inline style for
.attribution
to your styles.css file. It's considered as a best practice to avoid inline styles. -
You forgot to add accessibility features to your password input but you added accessibility to the email and name. A quick accessibility fix should get this done.
Once again, congrats on your solution. The only thing that works is more work
You can upvote my feedback if you found it useful. I'll appreciate :)
3 - @yaakouboxSubmitted almost 5 years ago
i ll be thankful for your notification about my code :)
@simeon4realPosted almost 5 years agoHey @Yaakoubox, Congratulations on your submission. Not everyone gets to complete challenges.
-
Firstly, I recommend working on your page's mobile responsiveness. You can improve it by using Media Queries. Learn more here
-
Learn to make your HTML more semantic, I mean, instead of having excessive generic
divs
you can split them intonav, header, footer, sections
etc. -
Don't use CamelCasing for your CSS classes. For example
.DivRight
should be.divright
or you can further to give your classes more meaningtext-box
since it's about the section with text. -
You can make use of flex-box to make your contents wrap when there's not much space. Flexbox will make your life easier. Wes Bos has an awesome course on that.
-
I suggest you start using BEM methodology for your HTML classes. See this referenced link: http://getbem.com/
Don't forget to upvote my feedback if you found it useful.
3 -
- @dagonmar183Submitted almost 5 years ago
This is my first project in Frontend Mentor. I made this landing page using anly html&Css, I used Flexbox to make responsive. Feedback is welcome!
@simeon4realPosted almost 5 years agoHello @Dani. I just previewed your website site. In addition to the recommendations by Rafael, I will suggest you reduce the font size on smaller screens. I'm currently on Mobile and the text are way too bigger.
One way I usually fix font-sizing is that I declare a base font size of
html { font-size: 62.5%; }
62.5% means that 1REM = 10PX and not browser default of 16px.
Then on small screens, using media queries, I set the font-size to
html { font-size: 50%; }
Now 1REM == 8px. This way, all the elements get resized automatically with just that one line of code since I use relative units. You don't have to go back to everywhere you declared
font-size:
to change them.Kindly upvote my feedback if you found it useful.
2 - @waynefoxSubmitted almost 5 years ago
All comments are welcome
@simeon4realPosted almost 5 years agoHey Wayne, I just previewed your solution and It's on point. However, I have a couple of recommendations.
-
Reduce the font size on smaller screen sizes. You can accomplish this using Media Queries.
-
Don't use IDs for styling your websites. Javascript uses ID and you can't apply an ID more than once. Instead, I suggest you make use of classes. Instead of
#submit { styles here }
You can use
.submit { styles here }
-
Embrace Cleaner Codes. Learn how to use the BEM methodology and learn about HTML5 Semantic markups.
-
To fix those accessibility issues, you should wrap each of your input with a
<label></label>
element.
Then you can add **aria-label="description of input" ** to your inputs.
You can upvote my feedback if you found my recommendations useful.
1 -
- @PleopleqSubmitted almost 5 years ago
It took me a loong ass time to finally finish this project. It was the most difficult i learn a lot. It not 100% accurate (I didnt know how to set up the background image). But hey, im still learning CSS. Its finally here! haha Happy New Year everyone.
@simeon4realPosted almost 5 years agoHello @pleopleq, Congrats on your submission. Just saw you know, I had a hard time completing the Fylo dark theme challenge as well. I used CSS Grid for the Desktop version but didn't know how to make it responsive so on Mobile, I switched the display to flexbox ๐
I have a couple of recommendations though:
- Wrap your input forms using the
<label></label>
element. Here's an example :
<label> <input type="email" name="name" > </label>
-
Add
arial-label
to your input forms. It helps with accessibility which is continuously growing on the web. Eg:<input type="email" **aria-label="email address"** >
-
Learn BEM Methodology:https://www.smashingmagazine.com/2018/06/bem-for-beginners/
-
Decrease the font size on smaller screens.
You can upvote my feedback if you found it useful.
2 - Wrap your input forms using the
- @silva-guilhermeSubmitted almost 5 years ago
Hi everyone, first of all it's a pleasure to be here sharing my stuffs and acquiring knowledge with all of you.
I have some issues when i was doing this challenge.
First, i don't know javascript yet to build beautiful interfaces, so it was kind hard to me (a novice in web development) to complete perfectly the site exactly the same as the example.
The hover effects don't work as it should be when i hover the mouse on it, especifically on the social media icons, that i used svg's and i dont know how to change the color of it even passing hours trying to do so :x
I really really enjoy doing this, and i hope you guys read my submit and could give me a feedback and tips to improving my code.
(Sorry if you are reading grammar errors, english is not my primary language)
@simeon4realPosted almost 5 years agoI have a couple of recommendations:
-
To get hover effects on buttons and links, add:
display: inline-block
to your .btn class -
Your webpage breaks on screens sizes => 1400px
-
Learn CSS methodology, preferably BEM. Learn how here: https://www.smashingmagazine.com/2018/06/bem-for-beginners/
-
Don't use SVG images as Social icons. Instead, use SVG icons. You can use icomoon for that. It will improve performance.
Upvote my feedback if you found it useful.
2 -
- @patricktouchetteSubmitted almost 5 years ago@simeon4realPosted almost 5 years ago
Nice one ๐ฅ. I'll submit my solution soon as well. Kudos Patrick
1 - @GO4ITJBSubmitted almost 5 years ago
Any pointers on the structure of the code and best practises??
Cheers :)
@simeon4realPosted almost 5 years agoFirst things first, Congratulations on your solutions ๐ฅ I'm yet to submit a solution to this challenge btw. Here are a few pointers, You can use BEM methodology for making your HTML more semantic. That way, instead of using generic class names like
txt-box1
you can useintro
,subscription
or evencard
Also, Instead of using generic divs everywhere, you can use semantic tags like section, nav, article etc
Finally, I would suggest that you give class names to some of your tags like h1. So you don't repeat yourself by using same declarations for many tags. For instance
.text-box-1 h1 { Styles here } // and later doing this : .text-box-3 h1{ styles here }
You can do it better, like this:
.heading__primary { styles }
Now you can use that single class for both text-box1, text-box-2 etc. Hope that helped.
Don't forget to upvote my feedback. Will be appreciated :)
3 - @maquindeSubmitted almost 5 years ago
Can I have some feedback on my HTML & CSS? I feel like my CSS could be cleaner...
Thank you in advance!
@simeon4realPosted almost 5 years agoYour solution looks good. About your HTML5 and CSS code, I think It will be better if you apply BEM - Block Element Modifier to your solution. It will make your markup more semantic. Also, I recommend using classes to style elements. for your headings, You can use a class like
.heading__primary { __code here__}
This article on SmashingMagazine will help with that.
Don't forget to upvote my feedback if you found it useful
2