search

labrocadabro / communitytaught

MIT License
78 stars 31 forks source link

feat: Modified Styling #37

Closed coltonehrman closed 8 months ago

coltonehrman commented 8 months ago

Summary of Changes

Styling changes across site to be more consistent in colors and smaller sizing as to not to look zoomed in. Added a max-width to the content of the app to not expand to screen sizes that are ginormous. This is a common practice across sites with good design.

Replaced the home page image with an image that speaks more to community and learning.

Color consistency was done mostly to keep the code consistent with colors, as you can see, some colors where re-used in the changes, but the intent is to stick to the twilight theme color and we can explore new colors to add to the theme later.

Home Page

home-page

Classes

classes-page

labrocadabro commented 8 months ago

Thank you for all the work you're putting in, I really appreciate it. However, in the future please submit an issue before a PR. That way we can discuss what you want to do and your reasoning for the changes before you begin work. It should help cut down on the number of changes you need to make. As for this PR, can you please give me a summary of the changes you've made and why?

Also a small technical issue: it looks like you've branched off of your previous PR rather than main.

coltonehrman commented 8 months ago

Thank you for all the work you're putting in, I really appreciate it. However, in the future please submit an issue before a PR. That way we can discuss what you want to do and your reasoning for the changes before you begin work. It should help cut down on the number of changes you need to make. As for this PR, can you please give me a summary of the changes you've made and why?

Also a small technical issue: it looks like you've branched off of your previous PR rather than main.

Makes sense. I was just having some fun and wanted to make a few tweaks to the styling :)

Yes, I branched off my other branch on purpose lol

labrocadabro commented 8 months ago

I'm going to close this PR for now. I agree the styling needs work, but the project might become more integrated into the 100Devs main site, in which case we'd want to restyle to be more in line with everything else anyway.

Second, this is really more like 4 different changes: text size, max width, homepage photo, and colors. And there may be a few other small tweaks mixed in as well. It would be better to deal with each one separately. I'm not going to change sizing or colors for now. Max width I agree, but I think xl is too small (there's a tradeoff here between how wide the site is and how much people are forced to scroll). 2xl should be fine. And the homepage photo, I absolutely agree with. Since there's nothing really to discuss at this point, feel free to submit a single PR with just those two changes.

My point about the branches was that if you're making two unrelated changes (and I don't see any overlap in these two), you shouldn't branch one off the other. It makes a cleaner commit history, plus can avoid some issues (like if the styling PR is ready to merge but the DX PR still needs work). It's not a huge issue here, just thought I should point it out.