Fixed issue#7 by using history.replace

Redd-Developer / howurls.work

Learn how URLs work by visually inspecting their structure.

https://howurls.work

MIT License

100 stars 9 forks source link

Fixed issue#7 by using history.replace #9

Closed manassarpatwar closed 4 years ago

manassarpatwar commented 4 years ago

Instead of NavLink I had to use divs, but I kept the class name as NavLink. Thats the only 'big change'

PS. Thanks for telling me that its open source on reddit!

vercel[bot] commented 4 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/redd/howurls-work/n28xp65zt ✅ Preview: https://howurls-work-git-fork-manassarpatwar-master.redd.now.sh

kettanaito commented 4 years ago

Hey, @manassarpatwar. Thanks for the changes!

I've tested it, and I see that we have lost the ability to highlight the active link. Do you think it would be possible to cover with the current changes?

manassarpatwar commented 4 years ago

I was using 'className =' which replaced the whole class list. Fixed it by adding the active class for the active url. I completely missed that the active url is highlighted.. sorry about that 😅

kettanaito commented 4 years ago

@manassarpatwar absolutely no worries! Thank you for such a cooperative fixes. It looks amazing. Please, let me rebase your branch and I'll merge it to master.

manassarpatwar commented 4 years ago

@manassarpatwar absolutely no worries! Thank you for such a cooperative fixes. It looks amazing. Please, let me rebase your branch and I'll merge it to master.

Sure thing @kettanaito! But before you incorporate the changes, I have a cleaner solution to this issue which restores NavLink but it essentially means that an anchor element will have an href and an onclick function. I feel it is unnatural to have an onclick function inside an anchor element. What do you think?

kettanaito commented 4 years ago

I've rebased the branch and provided a few adjustments:

Replaced div with button, to be a proper interactive element. Effectively, we are changing the page's state, but not navigation (hijacked by history.push()), so it falls under the button definition.
Composed the className for the button outside of the return statement of that component.
Styled the button as the link has been previously styled.
Dropped the font-weight: 600 on active links, since that renders weird in buttons in Safari (letter-spacing jumps visually).

Overall great changes! Thank you once more, @manassarpatwar. Welcome to contributors!

manassarpatwar commented 4 years ago

Awesome @kettanaito! The site now redirects much cleaner! :) Glad to contribute.

kettanaito commented 4 years ago

@manassarpatwar, please if you have any other ideas or suggestions, let me know in the issues. Your feedback is highly appreciated! You rock.