I'll be open to constructive criticism.
Victor Brito
@VictorBrito13All comments
- @victorad375Submitted over 1 year ago@VictorBrito13Posted over 1 year ago
Hi I checkout your and I would made some changes:
To the ever class I would use the grid to make the column layout and justify the content.
Also I would remove the margin-top to the img and the padding-right to the im class because it broke the layout
Finally add a media-query to improve the mobile layout.
Some resource: grid: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Grid_Layout
media queries: https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Using_media_queries
box model: https://www.w3schools.com/css/css_boxmodel.asp
PD: I also create a pull request to you repository
0 - @SharppegsSubmitted over 1 year ago
The key problem here was the buttons at the bottom of the page, particularly in the mobile view. They needed to be within the form element, so I gave them a fixed position at the bottom of the page, although they'll be in different positions depending on the browser. I had to move the 'monthy-yearly' toggler on the second page to fit them in better.
The general size and spacing of the elements was also difficult to get right. I estimated most of the them using margin and padding, but it would be better if they were more consistent. Maybe bootstrap would have been better here?
The functions work well, but maybe I needed fewer of them, or there are more efficient ways to get the same result ?
@VictorBrito13Posted over 1 year agoMaybe if you remove the position: absolute to the personal-info and selection-btns classes gonna help with the design
1 - @vanamos12Submitted over 1 year ago
How to center a box vertically?
@VictorBrito13Posted over 1 year agoYou can add this to your body:
This gonna make that the body takes the 100 percent of the viewport and the align-items gonna take the desire effect
{ min-height: 100vh; margin: 0px; }
2 - @ChidelemmanuelSubmitted over 1 year ago
am not sure of the mobile view pertaining to the container also i didn't do the navbar for the mobile using Js i haven't start Js
@VictorBrito13Posted over 1 year agoHey I saw your site and it is ok but I recommend you to remove the margin to the div with classes: 'all' and 'in-all' and use padding instead, this gonna make that the elements add a "intern margin" instead outside and this wont break your layout, also you can check this link: https://developer.mozilla.org/en-US/docs/Web/CSS/padding. And also you can use the "background-image"(https://developer.mozilla.org/en-US/docs/Web/CSS/background-image) property to set a image as background instead create 2 "img" tags.
0