ArdanaLabs / DanaSwapUI

Other
3 stars 3 forks source link

Landing page routing issue #70

Closed elite0226 closed 2 years ago

MatthewCroughan commented 2 years ago

@toastal Was your review unrelated to the actual point of this PR? I cannot tell whether you're reviewing the change, or if you are reviewing code that is not related to this change.

toastal commented 2 years ago

@MatthewCroughan Moving from an accessible <a> (or <Link> because React loves to wrap everything) to an inaccessible <span> is probably not the right solution.

toastal commented 2 years ago

@MatthewCroughan However, the _blank issue, while absolutely still an issue, it is currently in production and unrelated to the short scope of the routing issue. But being so tangential to the code, attributes on the same element, and that it affects the implementation of handleNavigate(), seems like a good time to grab it as well.

MatthewCroughan commented 2 years ago

@elite0226 Could you PR the correct solution? I need this to be fixed so that I can move on with my work, and currently this PR is the only fix that exists.

toastal commented 2 years ago

@MatthewCroughan

I pushed a number of fixes to the whole setup, namely using react-router-dom's <NavLink> which does the history and linkage automatically.

However, the styles need some fixing @elite0226

leomayleomay commented 2 years ago

QA feedbacks:

toastal commented 2 years ago

@leomayleomay Yeah, this is one of the severe downsides to using a SPA for a website

@elite0226 looks like there's some info in this boy: https://v5.reactrouter.com/web/guides/scroll-restoration

leomayleomay commented 2 years ago

Another issue found in staging:

leomayleomay commented 2 years ago

more issues:

the same will happen in Chrome, Firefox and Brave

leomayleomay commented 2 years ago

Can confirm the commit https://github.com/ArdanaLabs/DanaSwapUI/pull/70/files/5b837e9a094d35a981b9432d8e74fe6e0d08f40e..8845b0560a2fc073422beb16f7f73ad49fc51106 fixes the scroll to top issue

MatthewCroughan commented 2 years ago

Another issue found in staging:

* open https://staging-frontend-dashboard-dc1ece41.ardana.org/home in browser will result in a 404 page

* open https://staging-frontend-dashboard-dc1ece41.ardana.org/pools in browser will result in a 404 page

* open https://staging-frontend-dashboard-dc1ece41.ardana.org/swap in browser will result in a 404 page

* open https://staging-frontend-dashboard-dc1ece41.ardana.org/dana in browser will result in a 404 page

This is because of the react router. This was fixed in 96ff200f0819b12f641a096523cdb6e11a99ed20 but was broken in later commits.

MatthewCroughan commented 2 years ago

@elite0226 Whilst what you did fixed the browse-ability of the page, (clicking on buttons will not result in a 404), it will still 404 if you go directly to the URL.

leomayleomay commented 2 years ago

Can confirm the commit https://github.com/ArdanaLabs/DanaSwapUI/pull/70/commits/0ffd28a222b0b4f42effe8b77235ef674c9f20e1 has fixed the issue mentioned here

leomayleomay commented 2 years ago

@elite0226 either it's dark mode, or it's light mode, I could not see the logos rendered properly at the bottom of https://staging-frontend-dashboard-dc1ece41.ardana.org/#/home

Screen Shot 2022-05-19 at 7 42 16 PM