molLbach• 30
@molLbach
Submitted
The code is easy overall, it's obviously targeted at people who are learning css/html, the only difficulty lies on centering the div and playing with the box model to make things look like the design given.
Arkadiusz
@Arkadiusz-coder
All comments
- Arkadiusz• 200
@Arkadiusz-coder
Posted
Hi, congratulations with your first Frontend Mentor porject!
I have few suggestions: 1a. Your .attribution element should have valid links. "Frontend Mentor" is linked by https://www.frontendmentor.io?ref=challenge, and this is general address to the user's profile page. So when I clicked it's send me to my profile. It would be better if the link will send user to your frontendMentor profile. I suggest to replace this link with your https://www.frontendmentor.io/profile/molLbach 1b. there is no link behind your name, so I would suggest set link to your github profile, so people who want to know more about you would be send to your projects.
-
You used h3 element for the title of the page. I guess it's because the font-size of that element where more convenient to you but it is a better practice to use title elements according to their hierarchy, so always start with h1, never skip the number and adjust font size of the element with css. Because this elements are mostly for providing some order to your code.
-
It's also good practice to make some emantic html. So for example your .attribution element could be within <footer></footer> (and put at the bottom of the page); your .image could be within <picture></picture>; your text (h1 and p elements) within <main></main>;
I hope this will be helpful with your next projects :)
Marked as helpful
1 -
@Blaben
Submitted
I had difficulties while I was trying to create the mobile version. I would appreciate an explanation of that.
Arkadiusz• 200@Arkadiusz-coder
Posted
Hi, you have a problem with mobile version, because it is harder to make mobile version when the desktop version is already done. Good practice for making layouts in CSS is to start your project with mobile version and change layout with @media query for bigger screens, not other way around. So your first @media should have this: "(min-width: 450px)", not this: "(max-width: 450px)".
Marked as helpful
0- zHelga• 50
@zHelga
Submitted
How to improve the code? What mistakes are there?
Arkadiusz• 200@Arkadiusz-coder
Posted
Hello zHelga, that's nice looking page. I like very much that you have made possible to hide the "social media" section by clicking again on the arrow. Some people forget about providing this feature, and I think this is the best solution for user experience.
But I see that your mobile screen lacks a picture, and a picture on the desktop screen is not exactly of the proportions required for the challenge. Did you start your coding from the mobile screen? Because it's good practice in CSS and it is easier to change layout from mobile to desktop. I try it both way, so I know ;)
In your HTML you placed your .preview_img within the .preview class. It may be that this is the issue that make it hard to put image on the right place and result in the effect of disappearing of the image.
I resolve it in a way that my image is within class container like yours, but I hold the image within div that is separate from the rest of the component. I declare class "main" under the picture, and within .main I put the rest of the content. Thanks to that it wasn't problem for me to make the picture visible on top of the component.
The code is like that: ```````html`````
<div class="container"> <div class="drawer_image_container"> <picture> <img src="images/drawers.jpg" alt="" class="drawer_image"/> </picture> </div> <main class="main"> .... ``````````Although I suppose that if you will not make those changes according to above suggestion, but you will start your project from mobile, and then proceed to desktop, problem could also not occur.
Last but not least, to change the proportions of the picture I made width of the container smaller that that of the picture. Div in which I hold the picture has width property of 295px and the picture's width is 365px. In result the frame of the div covers some part of the picture and make visible only limited area of it. If you will do it, probably the rest of the component will change proportions and you will have to adjust it accordingly.
I hope that will be helpful. Good lack with this and future projects :)
Marked as helpful
0 - Brunasc7• 20
@Brunasc7
Submitted
Arkadiusz• 200@Arkadiusz-coder
Posted
Hello, this is nice looking layout, but there are two things that I would change:
I would put padding on your '.left' element or - to get the same result - to '.left-text > p', so the ratio of text to the container would look like more similar to the original.
In @media I would fix both '.left' and '.right' elements widths. Now the .left is smaller than the .right one. I guess when you delete current width properties on both and instead set max-width for 232px or 289px (those are approximate widths of those elements). If you give both the same max-widths the should look more alike.
Hope that will be helpful, Greetings :)
Marked as helpful
0 - Anupreet Bedi• 10
@pennyhofstader
Submitted
Arkadiusz• 200@Arkadiusz-coder
Posted
Hi,
your solution looks nice and works all just fine but design file provided by Frontend Mentor sagest that after a number is selected by user the circular area of the given number (your span element) should stay gray, unless user will change hers mind I choose another number. I will not give you my solution to this since it is good practice to figure it out by yourself, and I saw you are not far from this in your project, cause the color of the given span is changing, but only for a moment.
I've noticed that in your HTML file you left section provided by original file. I guess it would be a good practice to remove it to your style.css file, just for better order of the project.
And last one thing: you insert images in HTML by using <svg> and <path> elements, within which you have a lot of numbers. Are they necessary? In my solution I just used something like this: <img src="./icon-star.svg" alt="icon-star" class="star-image"> And that's works pretty well.
I hope this will be helpful. I would be glad if you have any comment on my projects here :)
Marked as helpful
0
