search

mithi / epic-react-exercises

Practical React exercises with detailed solutions.
https://epic-react-exercises.vercel.app
Apache License 2.0
161 stars 39 forks source link

🐞 Fix #116: Hide ugly scrollbars on Windows. (At least in Chrome) #121

Closed r002 closed 3 years ago

r002 commented 3 years ago

Hi @mithi -- So I finally beautified the scrollbars for Windows (well, Chrome) this morning. (Lol, I thought this was gonna be a ten-minute fix.... but nope! 🤦‍♂️) Anyway, it was fun! 👍

The "scrollbars bug" fix:

image

And when the screen resizes, I took care of giving .main scrollbars in the @media section:

image

There was one tricky thing that took me a literal hour to figure out, btw: In main.js in MainGrid you'd hardcoded overflow=auto which had been overriding the css that I'd been trying to set in Style.module.css. Out of curiosity, was there a specific reason you'd hardcoded that there? I needed to delete that bit or else this would happen:

image

With the overflow=auto hardcode removed, it now looks like:

image

But in Edge, it still looks awful. I'd followed the below guide but it didn't work so I'm giving up on it. Sorry, Edge people! 🤷

PS. It me a few tries to get it right, so if you do merge my PR, you'll probably want to do a "squash & merge".

References:

vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/mithi/epic-react-exercises/7Lim7i8khUEc5FzWDutW1PNWkz7M
✅ Preview: https://epic-react-exercises-git-fork-r002-fix-116-mithi.vercel.app

mithi commented 3 years ago

@r002 Thanks so much for your effort! This is great! I made a couple of changes, let me know if it looks good on windows, before I merge it. Thanks again!

r002 commented 3 years ago

@mithi The code looks beautiful in Windows Chrome! 💯 I like the thinner scrollbars! 🙂

Basically, I tested two modes-- looking at in full-screen: image

As well as mobile: image

One question for you though: I noticed that in your Make nav buttons closer together change, you directly edited the SqaureButton React component in components/main/navbar.js. While that definitely works, I'm curious: Is there a specific reason you did not create a new "Nav Button" else if block inside of const SqaureButton in components/button/index.js? Or (maybe even better): Create an entirely new NavButton component that extends SquareButton but then just sets the { margin: "5px 10px" }?

Generally, I just get nervous when I see CSS getting manually set in the style='....' attribute inside of JSX. Like, I enjoy separating all styling concerns only to be the .css file OR at the .js level inside the actual component (eg. SqaureButton).

Does that make? Just curious about your reasoning.

mithi commented 3 years ago

One question for you though: I noticed that in your Make nav buttons closer together change, you directly edited the SqaureButton React component in components/main/navbar.js. While that definitely works, I'm curious: Is there a specific reason you did not create a new "Nav Button" else if block inside of const SqaureButton in components/button/index.js? Or (maybe even better): Create an entirely new NavButton component that extends SquareButton but then just sets the { margin: "5px 10px" }? Generally, I just get nervous when I see CSS getting manually set in the style='....' attribute inside of JSX. Like, I enjoy separating all styling concerns only to be the .css file OR at the .js level inside the actual component (eg. SqaureButton).Does that make? Just curious about your reasoning.

Oh no, you somewhat opened a can of worms here 😅. I hope you don't mind the huge wall of text below!

Disclaimer: Note this is just an opinion, I'm not saying this is the best way to write this code, these are the things that go through my head as I was writing it... there are always tradeoffs! Don't get me wrong, I also like putting the styling inside the actual component... but there are other considerations here... (other than I can't put styles in a class because of specificity, it won't be able to override the styles inside the SquareButton component)

(Also notice that I slightly refactored NavInner, without making a new component (I double down on that)...I hope you like it 😅)

https://github.com/mithi/epic-react-exercises/blob/57e5cdf83e0ba9845f1a2f80a56bc565fab055f2/components/main/navbar.js#L33-L39

Adding an if-else block is not a good idea...

The SquareButton is intended to be a general reusable component... Taking a look at the source code of SquareButton

https://github.com/mithi/epic-react-exercises/blob/9af49d58febe6bd1f27f439ab52aa7e87b027b65/components/button/index.js#L119-L121

You'd observe else if block is only for checking the side prop which can either be a falsey, large, small or some size like 15px for example. You might realize now that wouldn't make sense to add additional specific cases like NavButton or PaginationButton or ThemeMenuButton as it would go on and on and it goes against the api...

I read this

I have often found myself writing my API before I have even decided how I'd like to interact with it. After watching this series, I have started to write what I would expect my API to provide before actually writing it.

And I realize that I'm guilty of this, so now I try to think about the api first before creating components!

Yes, I thought about NavButton

because that's how I approached the reactMenu buttons the react menu As you can see here:

https://github.com/mithi/epic-react-exercises/blob/9af49d58febe6bd1f27f439ab52aa7e87b027b65/components/main/react-menu.js#L6-L16

But there are other considerations

duplication is far cheaper than the wrong abstraction - Sandi Metz

Here's some highly recommended reads/talks about this you haven't already!

Helper methods are object junk that we need to recreate and compare for no purpose other than superficially nicer looking syntax - Dan Abramov

React has a powerful composition model, and we recommend using composition instead of inheritance to reuse code between components.

It's nice to read and watch about Composition vs Inheritance

About separation of concerns

I'm not sure if you've tried tailwind but here's a quote from their site

Now I know what you're thinking, "this is an atrocity, what a horrible mess!" and you're right, it's kind of ugly. In fact it's just about impossible to think this is a good idea the first time you see it — you have to actually try it. But once you've actually built something this way, you'll quickly notice some really important benefits.....

Read more: https://tailwindcss.com/docs/utility-first

I know that not a lot of people may agree with it as you can see in the discussions from hacker news

But based on my experience...

When you realize how productive you can be working exclusively in HTML.... working any other way will feel like torture.

That's all! Thanks for coming to my Ted Talk!

r002 commented 3 years ago

Wow... this is amazing. Thank you so much for writing this up! This is... a lot. I'll need some time to properly go through and think through all of this. I've never looked into Tailwind CSS but now I will!

(Just from the bit I have read, it's already resonated with me a ton though. Eg. I am super-guilty of making tons of nonsense CSS classes just to style stuff. 🙄)

Thank you so much for open-sourcing your project though! It's been hugely educational for me to just read through your codebase to see how you do things. I'm not proud of it, but for years I just worked solo and mooched off of other people's projects (npm install ... !, stack overflow!) without contributing anywhere. But recently, as a resolution for 2021, I've decided to try re-entering society. 😅 So instead of just spinning up new projects, I've made it a goal to scour Reddit and Discord, find existing projects that look friendly, and then try contributing. It has been so much fun! Your project was the first I made a PR to; so thank you for making it such a welcoming experience! 🙏

(This sounds weird, but I've actually (just recently, as ridiculous as it sounds) realized that reading code is a legit skill. Like, the ability to paradrop into a foreign territory, find the entry point, build and run locally, and then reason what's going on is a bonafide ability.)

Btw, have you considered enabling the Discussions tab in your GitHub project? There are some general questions I'd love to ask you (if you're inclined, of course). And other folks who might be interested in contributing may also be curious as well. For example, looking through your history, I noticed:

image

And so not knowing a thing about Vercel or NextJS, I literally spent an entire day exploring that whole ecosystem. (Haha, I am the proud owner of a Vercel next-js-blog now! 😄) I'd love to know what you think about NextJS though. Personally, when I started getting back into webdev earlier this year, I planted my flag in the Google Firebase camp. I was impressed by their dev tooling (local auth and Firestore emulators!) and they also offer "preview channels" for PRs. But as always, I'm always curious what the landscape looks like and about other people's experiences with other tools/platforms. Always feels like 'the next big thing' is afoot and it's hard to keep up. 🤦‍♂️

PS. Btw, you might laugh, but back to the 'Nav Button' topic-- Years ago, on a previous codebase I'd worked on, we actually had a single "Button Factory" class that was responsible for just giving all of the buttons. Like, you'd pass in the type of the button you wanted and, poof!-- the button would just appear. The code was astoundingly ugly (hundreds of lines of if/else blocks) but I remember thinking at the time, after actually using it-- "well, there are no dependencies!" It actually gave me a ton of confidence being able to add a new button or restyle an existing one bc everything was literally in a single place. Ergonomics, I guess, is what this is called nowadays? What a world; everything's come full-circle, in a way! ⭕