@FMB237
What are you most proud of, and what would you do differently next time?
I'm proud i learn how to manipulate multiple DOM elements
What challenges did you encounter, and how did you overcome them?using Multiple elements
P
Katrien Schuermans
@katrien-sAll comments
- P@katrien-s
Something I just want to point out: what's up with the capital letters in your css and javascript?
The convention is:
- in javaScript: camelCase your words. Which means, first letter is not a capital
- in css: no capital letters
It makes your code look messy.
There also seems to be double code. You can remove
(min-width: 48rem):@media (min-width: 40rem) { .Faq-Accodation { height: 70vh; width: 70%; } .answers p { font-size: 1rem; } } @media (min-width: 48rem) { .Faq-Accodation { height: 70vh; width: 70%; } .answers p { font-size: 1rem; } }Also, in all your media-queries, the
.answers pgets the same font-size. It'll work if you would only add it to(min-width: 40rem)as it will be used for all screen-sizes with a minimum-width of 40 rem..answers p{ font-size: 1rem; }Aside from this, your exercise works and looks good. So I'd say just keep on coding though pay a bit more attention to how you write it. Happy coding!
- @Farzane2630P@katrien-s
Hi, it overall looks good but there's some minor issues.
I can click on all buttons, have them all highlighted before I click on submit. You should limit this to have only one button highlighted.
It's advised to combine all css in one file. There's no need to split it up. The same for your javascript. You can have it all in one file.
There is also no need to set a width on your body. It always takes by default 100vw.
Why did you use
!importantso often? Check also if by adding all your css to seperate classes, you maybe wrote too much css.For example:
.icon-wrapperand.thanks-icon-wrapperseem to have the sameflex-styling which you could maybe put onsectioninstead of adding the same code to 2 classes, that have the same use/outcome.But great job on this one. The Javascript works well, and it does what it needs to do. Have a look at the things I pointed out, and keep coding.
Marked as helpful - P@Kiara523What are you most proud of, and what would you do differently next time?
It was fun making it. I think I can do better with class names, I want to improve on that. Also there are different ways to approach this challenge.
What challenges did you encounter, and how did you overcome them?I didn't encounter major challenges, but I've learned more about grid.
What specific areas of your project would you like help with?I want to improve naming classes in html. I think that a good markup helps a lot.
P@katrien-sNot much to add as this looks good. Though, watch out for typing errors. You managed to spell
socialin various ways.Having myself also just finished the exercise it shows pretty well how different JavaScript-approaches lead to the same result.
Happy coding!
Marked as helpful - @a5420bcP@katrien-s
Not that much to add actually. Your solution looks really good. Are you going to continue writing your js and css inside your html?
As for your image, look into how to use
<picture>to have responsive images instead of showing and hiding mobile and desktop image.The
paddingon yourform-containeris not equal on both sides. You have set apadding-inline: 3rem 2remwhich really shows on mobile view. But it also gives too much white space on desktop view.There's something really strange here. When I look at your code on Github and the code of your webpage, I see 2 different solutions. Your webpage does not have inline styles and JS. It shows the too much padding on 'form-container' while on Github it says the padding is on
detail. Are you copying in other people's webpages? If so, I stop giving feedback here as I am not looking at your solution but someone else's. - @lakshmanan1205P@katrien-s
Your solution is like looking at a squeezed version of the exercise. There's a few things that I immediately to be odd.
- Don't set a width on
body. Your body-element is that entire visible in your browser. That always needs to be a full 100%, which it is by default. And which you don't change. In this exercise you did it obviously to have the component have the right width. But, that component is in yourmain-element. Guess what is ok to do with themain-element... If you want to change the width do it here. - use
widthon yourimginstead ofmax-inline-size, and add anobject-fitto have your img fit nicely. Read the article on MDN:
img { width: 100%; height: 100%; object-fit: cover; }Did you have the Figma-file to work with? As your typography for your h4 element seems a bit off. Besides, having an h5 on the author-name isn't really correct. You could write this as a p, but add a bold-class to it.
This is a tough exercise. I didn't read through your javascript because it actually looks really complicated while this is a beginners-exercise. So I'm wondering why your JS-solution looks this complicated. Why are you not using the camelCase naming convention?
If you pay a bit more attention to the design I think you're well on your way to be a good developer. Keep on coding and more importantly, enjoy it :D.
Marked as helpful - Don't set a width on
- P@SKszymekWhat are you most proud of, and what would you do differently next time?
I'm proud of my self that I end up with that result. Next time I have to learn more about responsive design. I make It but I'm felling not comfortable with It.
What specific areas of your project would you like help with?That's my first landing page so I would be gratefull if somebody check my code. I need help with queries and withds. To be honest I don't know when exacly use percentages and when max-width. Thank's for any advices.
P@katrien-sHi, you nailed it! There's some little things here and there, but overall looks good. Also your HTML reads very well organized. Same for your CSS.
Small error in your organisation of HTML could be that you entered your
headerandfooterinmain. It's actually possible and sometimes done. But, for HTML to be semantic and in terms of accessibility, you also need to have aheaderand afooteroutside ofmain. A basic HTML-structure looks like<header></header> <main></main> <footer></footer>As for your question about responsiveness. I do see looking at your code you're a bit lost in all the information. First of all, whether you use
max-widthormin-widthdepends on whether you style starting mobile first of desktop first. You've been using a lot of media-widths, which is not that necessary. You can get by by using 2 or 3 settings.You've been indeed using percentages a lot. I did that too when just starting out, but it's again not always necessary. You actually don't need to declare a width that often.
Kevin Powell has this awesome course on Responsive Layouts. This is going to help you a lot.
Marked as helpful - @ephraimdjeketWhat are you most proud of, and what would you do differently next time?
This challenge really helped me understanding grid better. Will try and use grid more often.
What specific areas of your project would you like help with?Do you see where I can improve? Please share :)
P@katrien-sNot much to add, cause this looks good.
Though, did you use the correct font-sizes? Cause the text is really small and especially on mobile view. And this line:
.testimonials-container article:nth-of-type(3) .testimonial-content .graduate-wrapper smallof which there are quite a few, that is unnecessarily clunky. Why not create atext-darkor atext-lightclass and add it to thesesmall-texts. Because once you start typing out 5 selectors to change a text-color, you should rethink and find a more logical solution to what you're doing.I don't immediately have more to add cause your solution rocks. Happy coding!
Marked as helpful - @Davilucca22What are you most proud of, and what would you do differently next time?
Do que você mais se orgulha?: em conseguir fazer o projeto em pouco tempo
o que faria diferente na próxima vez?: implementaria javascript.
P@katrien-sIt looks good. But, there's quite some oddities in your code.
In your
headeryou have toh1-titles. Even if it's possible to write that, it's very bad practice. There can only be 1h1on your html-page and it has to be the main title of your page. It's tough to know which one it'd be here. You can style any text-element to look like anh1without it being it.You've put a
display: flexon yoursection. But it only has one child, adivso it is of no use here. Why did you use aflex? What did you want to achieve?As for the
gridon yourmain, consider usingtemplate-columns: repeat(4, 1fr)instead of33.33% 33.33% 33.33%. It's actually the same, but it will save you some math and increase readability.Why did you set
widthon your text-elements? Why is there aoverflow-x: hiddenin your css-reset?I think, eventhough it actually looks like it should, you might want to go over your code again. Move your image-div outside the div with all the text. What does
sectionlook like then, with and withoutdisplay: flex. Remove thewidth: 100%on your.image. You want that to be on the actualimg, not on its parent. Just rethink and shuffle the elements and see what happens.I know. it's annoying because you finished this exercise and it looks good. But once you move on to tougher exercises, you'll be running in 'CSS is hard'-moments and the sooner you address these moments, the easier writing CSS becomes.
Don't let this discourage you. The code you've written is very structured and readable. You followed the design and created exactly what it should look like. Which is awesome. So, continue coding, you got this.
Marked as helpful - P@taceseptP@katrien-s
This looks good. But, can you add in the correct
font-familyon yourh1andprice? And fix theborderof yourbutton? While you're at it, addcursor: pointerto get the pointer when hovering the button.Happy coding!
Marked as helpful - @morisonfrederickP@katrien-s
This isn't finished right? It sure doesn't look like the design. Was this intended?
Your HTML reads ok. The CSS is readable to, but since it's incomplete, it's hard to say what it would look like once you finish the project.
Anyhow, I see you've been adding a
width: 100vwon your body. That's not necessary. It will always be 100% iof your screen-width, unless you change it. But there is no need to change width of abody-element.Happy coding!
- P@Oliko136P@katrien-s
This looks good. Not that much to add actually.
But, if you want to up your responsiveness. There are a few ways to no longer need to adjust your elements-width in a media-query but do it in one line on the element itself using
minormax. To get yourself started, check the video 'These CSS PRO Tips & Tricks Will Blow Your Mind!'Which means you're
.profile-code could end up looking like this:.profile { width: min(327px, 456px); }Since you had added a 3rd width, you probably want to use
clamp()to define your width. It's also being explained in of the video's on that YouTube-profile.Happy coding!
Marked as helpful - @vijay1marojuP@katrien-s
Honestly, no matter which screensize I look at your project, it's all messed up. When I look at your code, I have tons of questionmarks and 'why did you do it like this?' popping up. It's very hard to find a where to start giving you feedback.
-
You centered the
.main-cardusingposition:absolute. You could do that, if you know how positioning works. But it is also one of the hardest things to learn as a beginner so it's advised to use it as least as possible. Tip: The fastest way to center vertically & horizontally with CSS -
A lot of your elements have a
transformwhich you've used to put them in position. You could do this a lot more simple usingpaddingon yourmain-card. -
If you declare the
font-familyon yourbody, there is no need to declare it anywhere else again, unless the item has a differentfont-family. -
When writing HTML, id's are most commonly used for when you need access to that element in JavaScript. For CSS, use classes
Difference Between IDs and Classes While classes and IDs may seem similar at first glance, they have distinct purposes and use cases:
Classes: Classes are designed to group multiple elements together. They allow you to apply shared styles or behaviors to various elements throughout your webpage.
IDs: IDs, are used to uniquely identify individual elements. They provide a way to target specific elements for styling or manipulation.
We've all been here, messing up and being a bit clueless. We only learn by making mistakes. So surely don't give up, because coding is fun.
Have a few more lessons and try again. One of the better online tutors is Kevin Powell and he just released an HTML & CSS for Absolute Beginners
-
- @lordymarcWhat are you most proud of, and what would you do differently next time?
Today I learned how to create a README, Github Pages and Figma (sorta kinda). Nicee. Thanks for reading.. I'm too insecure to hear any feedbacks and tell everyone and their mama where i screwed up bye
What challenges did you encounter, and how did you overcome them?Life has been rough lately...mmmh yeah
What specific areas of your project would you like help with?Everything one could think of
P@katrien-sGive yourself a pat on the back, cause this looks good. Your code is nicely structured and readable. Very clean.
Though I did wonder why you used
row-gapon your body until I noticed it was to create a gap between thecontainerandattribution. You could've used a simplemargin-topor-bottomhere. Though no big deal in this little exercise.Happy coding!
Marked as helpful - @LexTarasovP@katrien-s
Check this Kevin Powell video: Are you using the right css unit and have a browse through his more recent vids.
- @markvarga21P@katrien-s
It has nothing to do with using
rem. As you're supposed to be using them when you want to make things responsive. This seams to me to be the culprit:width: 17svw;on your#qr. Which on smaller screens you give a75vw. This means on tablets and smartphones, the qr will take up about 75% of the screen width. But on desktops, it will shrink down to 17%. What you need to do here is, give yourdiv id='qr'a max-width. There is various ways you could write this, but an example could be:* { box-sizing: border-box; margin: 0; padding: 0; } #qr { width: 95vw; max-width: 350px; margin: 0 auto; }Here we tell the div you need to be 95% of the viewport, until your maximum 350px wide.
Are you familiar yet with how the box-model works in css? CSS Box Model And what the purpose is of this code I added:
* { box-sizing: border-box; margin: 0; padding: 0; }Also, don't use
divto write headings or text. There are html-tags especially for text. HTML Text FormattingMarked as helpful - @Joel12rP@katrien-s
Answer to your question: don’t put all your li-content in a span. Just wrap a span around the words that need to be bold. All the rest of the text can just be inside the li: ´<li><span>Total: </span>Approximately 10minutes</li>´ If nothing needs to stand out, don’t use span. It is ok to have text inside an li-element. Also in the <ol>, there is no need to type the numbering yourself. It happens automatically.
This is a helpful article: https://css-tricks.com/everything-you-need-to-know-about-the-gap-after-the-list-marker/
Marked as helpful - @Mukulvjain1P@katrien-s
Desktop looks good. Mobile is where things really go wrong. I remember myself struggling quite some with this exercise, so don't feel down for not entirely achieving the result. It's part of the learning process.
It seems to me you got yourself in a lot of trouble with the extensive use of setting widths and heights using %. Are you familiar with working with flex or grid?
As for the image, you have been given 2 images: one for mobile, one for desktop. You need to use those. Have a look here to understand how te be using them: Responsive images
I don't understand why you set this width & height on your container. If you're going to use full-width, you don't need to specify it, as your webpage will always take up full-width. The height might be more usefull on your body or html-element, if you're using it to center your container.
.container { width: 100vw; height: 100vh; }Also specifiying height on your main, seems overkill. A flex-element will always take up all the space it needs. And it's very rarely necessary to set a height.
Are You Making These CSS Height Mistakes?
One of the most common CSS issues people run into
Try again. Look into how using flex or grid. Whether it's necessary to set width and height. And have a little round of applause for yourself when things look good. Happy coding!
Marked as helpful - @Ezra003P@katrien-s
Hey, what you did looks good. Though when you finish an exercise, fill out the readme-file as instructed. It's important to create a habit from the beginning, as readme-files are vital later on in your career.
- One thing that I immediately noticed, is the use of nothing but
<div>s in your html, beside the<Main>(which has no need to be capitalized). If working with text, use the correct html-tags. As it's important to write your html readable for screenreaders. It's vital to use semantic html correctly. (HTML tags for text)[https://flaviocopes.com/html-text-tags/] - Why did you split up your css in 3 files? For a small project like this, there's no need.
- You declared a
cursor: pointeron the hover-state of.course. You can move that to.courseitself. - What is this url-reference doing here?
cursor: url(/assets/images/cursor.png), pointer;(The assets folder won't open on GitHub so I can't see what you're linking to) - The font you used, 'figtree', hasn't been installed?
- I suppose by using
margin: 100px 545px 100px 545px;on your<main>you were trying to center main in the middle of the page? (How to center a div horizontally)[https://www.youtube.com/watch?v=ULVu2VNM_54] - There's also no need to set a width on your body. Your body will always want to take up the full width of the browser window.
There's room for improvement, but this sure already looks good. It shows you have an eye for design vs. code. Have a look at what could be improved here and move on. There's lots more fun projects waiting for you. Happy coding!
- One thing that I immediately noticed, is the use of nothing but
