@Namonaki0
Posted
Hi Anthony, well done for completing the challenge. It looks really good.
Is Bootstrap an acceptable end all framework for a project in the professional space? - Yes, tonnes of sites use it every day, and many of them for commercial purposes. You just might want to check the comparison between various licenses. You can check some of these websites.
You can also get rid of any code that's commented out if it's not serving any purpose, like the attribution bit before the Bootstrap CDN, along with the CSS that was left in the <head>
.attribution { font-size: 11px; text-align: center; }
.attribution a { color: hsl(228, 45%, 44%); }
You don't need to have different ids for each button. You can set the background-colour to transparent on hover which will then show the colour that's behind each one. I can see at the moment you have background-color: hsl(0, 0%, 95%) for each individual button id - #sedan-button, #suv-button, #luxury-button. Try to adopt the DRY principle and have the background-color: hsl(0, 0%, 95%) and border: 2px solid hsl(0, 0%, 95%) in the .button instead.
You can also style the h1s all at once without the need to have an id for each one of them - #first, #second, #third.
I hope this helps a little bit.
Marked as helpful
@anthonyplicata
Posted
@Namonaki0 Very helpful! Thank you. I'm going to have look at some of these websites.
I feel like its a good habit to get into remembering to remove a lot of that because I am really just forgetting about the commented out code once I feel finished. Noted!
So I got rid of the ids for each button and applied the ccs you suggested. It worked well for the hover. I had a problem with maintaining the color of the text since color: transparent takes on the background color of the button not the panel. Instead I added the color to the respective class and made separate class selectors for just the text color. Doesn't feel like it adheres to the DRY principle but it takes up much less than the ids I had!
I'm still figuring out when, where, and how best to use ids. I can see now how overusing them becomes redundant. I moved all the styling to the h1 element selector, but the font-size: 2.5rem wouldn't work without adding !important to it. Why would that be? I can't think of anything that would be overruling it.
@Namonaki0
Posted
Hi Anthony, I forked your repo and made some changes, I hope you don't mind but I sent you a PR named 'Code Review' - link here - just so you can have a look at some examples of how to perhaps condense your CSS by structuring your HTML differently. By no means I'm implying that my way is better than yours I'm simply providing alternatives that might help you in the future. No need to merge that branch into main, it's only for reference. I focused mainly on structure and readability so you might want to still look at things like font-size, etc...
In short:
- Added CSS variables for the respective colours since they are shared between the background and button text colour. The same goes for the font-family type. This way if you need to change anything in the future, you can do it in one place instead of several selectors.
- Got rid of the title ids and moved
font-size: 2.5rem;to the h1 selector in the CSS. I also removed the div after the<main>tag and moved the.rowto the next available<div>. - Media-queries - no need to rewrite the rules that were outlined above. Only write rules that will overwrite the previously defined ones.
I moved all the styling to the h1 element selector, but the font-size: 2.5rem wouldn't work without adding !important to it. Why would that be? I can't think of anything that would be overruling it. - The answer might be in specificity in CSS.
I hope this helps.
Marked as helpful
