Converted NavButtons component into TypeScript

sicin commented 2 months ago

Changed the NavButtons component into TypeScript. Index is apparently not a valid prop of History, altough it obviously exists on the object, that's why the 'as' keyword was used.

nuki-chan[bot] commented 2 months ago

Lines 1-47 in deleted `index.js`

Oh dear, someone's been coding like it's still 2015. 😆 I see we're throwing out the old class components to make room for those shiny, new functional components with hooks. Goodbye constructor, bind, and opaque this references! Sayonara!

But wait! Before we sail into the sunset with our enlightened code, Nuki noticed a couple of nitty-gritty details. The links here use href='#' which can cause the page to jump to the top when clicked. That’s no good, we don't want to give our users a mini heart attack every time they try to navigate, ne?

Lines 1-35 in new `index.tsx`

Oh look, kawaii functional components in their natural habitat. 🌸 React hooks are spreading their wings longer than Nuki's last anime binge.

Unused useEffect Dependencies:
```
useEffect(() => {
setBackButtonEnabled(history.index > 1);

setForwardButtonEnabled(history.index < (history.length - 1));
}, [history.index]);
```
Here, you're adding history.index as a dependency, but you may also need history.length unless you are 10000% sure (like certain you won't drop your ice cream 🍦) that history.length doesn’t change without history.index changing as well. It's generally a good practice to include all state or props that's being used within useEffect. But hey, Nuki is here to suggest, not to judge. (Maybe a little to judge 😽).
Class Names With Conditional Logic: The way you're setting class names is cute, but it could be clearer. How about a sprinkle of template literals?
```
className={!backButtonEnabled ? 'disable' : ''}
```
Kirei ne? It reads better than using a logical AND, and is less prone to accidents!

Make these changes if you feel like making Nuki proud! 😸 Remember, code reviews are like group projects; everyone has to play nice with each other, but Nuki's advice is not the be-all and end-all. Keep growing, senpai!

By the way, a shiny ✨ new PR walked into the dojo without a trace of tests. We wouldn't want our fresh code to catch a cold or, Kami forbid, a bug 🐛! So don't forget your tests, dear developer!

Now go forth and refactor! (ﾉ◕ヮ◕)ﾉ*:･ﾟ✧

sicin commented 2 months ago

After some thinking I've realized that Nuki-chan is right. PR fixed.

nukeop commented 2 months ago

Thanks, this looks pretty good. In the future instead of force pushing there's nothing wrong with just committing over your original work so we can see what happened at each step.

nukeop / nuclear