This was fun to make. A little "head scratching" on js but it worked out in the end.
Nenad Cosovic
@nenadmneAll comments
- @samy-ardSubmitted almost 2 years ago@nenadmnePosted almost 2 years ago
Good solution.
Check adding number and backspace in the middle of the card number input.
0 - @ladaspcsnSubmitted about 2 years ago
I feel like this one was pretty easy to do, since it didn't require any adjustments for mobile version. The hardest part for me was making an overlay when hovering over the image, but I managed to do it by using absolute positioning and z-index.
I would really appreciate any comments or advice on improving my solution.
@nenadmnePosted about 2 years agoYour solution is looking great with consuming everything what was asked from this challenge.
Only thing i would suggest is wrapping classes with same attributes under 1 line, like this:
body, .img-overlay, .price-section, .author { display:flex; }
Alse same could be done for
justify-content: center;
oralign-items: center;
Great solution, keep up =)
Marked as helpful1 - @earlyronnieSubmitted about 2 years ago
Any feedback is appreciated!
@nenadmnePosted about 2 years agoYour solution is very elegant and simple. I loved it =)
Here are few suggestions:
* { margin:0; }
Consider putting bellow
margin
a global parameter offont-size
, and then on rest of the classes userem
as font-size matter. Also puttingfont-family: 'Outfit', sans-serif;
would work great here if its only 1 family for whole page..text h1 { font-weight: bold; }
Here instead
bold
you could have usedfont-weight: 700 (or 600);
so it would look same as challenge heading :)Great solution by the way =)
Marked as helpful0 - @ShaftJnrSubmitted about 2 years ago
how to keep the cart icon on the same line as the "Add to cart text"
@nenadmnePosted about 2 years agoHello, i look into code
<button> <div class="button_text"><img class="cart_icon" src="./images/icon-cart.svg" alt="cart"> <span>Add to Cart</span> </div> </button>
From this html part, remove div that is inside button, and put button class
class="button_text"
instead. Div in not supposed to be inside button.After that go in css and type
.button_text { text-align:center}
. This will center img and text in button.To make space between them just margin it in css.
Keep up =)
0 - @jmzarate09Submitted about 2 years ago
I'll appreciate any feedback. Thank you.
@nenadmnePosted about 2 years agoLooks like a nice and simple solution.
Only thing i would add is that icon position (eye in the center of equilibrium photo).
<div class="icon-img"> <img src="images/icon-view.svg" alt="icon-view"> </div>
Try to either use
z-index
or pseudo::before
to fix its background color, as it should be white on hover.Happy coding =)
Marked as helpful0 - @kacper-reimanSubmitted about 2 years ago
Any feedback would be apperciated
@nenadmnePosted about 2 years agoNice solution there. I spent some spare time to look inside your code.
1.I think when switching from mobile to desktop version you should quit using % and give strict width and height. Its recomended for most sites to not get width over 800px so our eyes dont get tired moving around screen. My screen is so 1500+ so your solution was very much streched.
2.Instead writting
font-wight:700;
ortransition: 0.5s;
under every class, you could wrap them all under 1 place like thisbutton, .price span, .attribution { font-wight:700;}
. When have same style for more then few classes, its always best to wrap them under 1 line so you change them easier and your code looks less stacked.Nice work and happy coding =)
Marked as helpful1 - @ALAS08Submitted about 2 years ago
All feedbacks are welcome!
@nenadmnePosted about 2 years agoI like your coding style, you kept it pretty simple. I noticed few things that you may consider in future coding.
1.What would made it even simplier is that, if you have more classes with same attribute, you can wrap them all like this
.sedans, .suv, .luxury { width: 202px; height:350px}
. So instead repeating 3 times, you got their box dimensions on one spot. Easier to change them after, if need.2.Also this line
@media screen and (min-width: 992px) { .main-container {display: flex;}
is not needed becuz you already stated thatdisplay:flex;
in mobile version.Nice solution, keep up =)
Marked as helpful0 - @mbonamensaSubmitted about 2 years ago
I am happy I have been able to get this far. I struggled a lot with the background positioning of the patterns and then with the illustration woman and box. I decided to show what I have now and ask for help on how to properly do this.
My issue with the illustration for the mobile was having it stick to the top of the container even with different screen sizes. I used percentages in the absolute positioning for this but it did not seem to work past around 300px screen sizes and below.
The illustration box also does not stay in one place. I tried using
position: fixed
and it did not work as I wanted it to. I'll appreciate help on my positions for the different screen sizes. Also, feedback on accessibility is welcome.@nenadmnePosted about 2 years agoContainer has
overflow:hidden
so left half of your box is hidden outside of container, because its position is relative to parent who has hidden overflow. Simple solution would be that parent for the box image is body, so its wraped out of overflow.body::after
There is probably 20 more solutions arround just pointing out the most newbie one0 - @HyoganSubmitted about 2 years ago
-Just please take a time to look at my code and tell me what i've done wrong
Thank in advance@nenadmnePosted about 2 years agomain { padding: 10px 10px 10px 10px; padding: 1%; }
1.You are giving 2 padding attributes. If all paddigns are 10px, just write it down once.
2.When you are doing same attribute to more classes like
display:flex
, for example, you could just do it like this:.post-block, .post-section, .introductory-section, main , body { display: flex }
So instead writing it in 5 lines, in 5 different classes, you write it down in single line.Nice project, keep coding =)
Marked as helpful0 - @msyahrezaSubmitted about 2 years ago@nenadmnePosted about 2 years ago
Hello, i had some spare time so i look into your code and shere few thoughts.
h1 { font-weight: bold; }
// I would use just
font-weight: 700;
as you did in.category
further bellow. //@media only screen and (max-width: 700px) { .prices { display: flex; align-items: center; }
Here i saw you repeated some of the atributtes from previus code. When using @media you should write down only code that is changing further.
Last thing i would suggest is that instead writting for example
display:flex
, for every class, you just tight them all under same line:.one, .two, .three, .four { display:flex; }
nice project and happy coding =)
1