Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Apply routing fixes in browser #179

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Apply routing fixes in browser #179

wants to merge 1 commit into from

Conversation

ali9821
Copy link

@ali9821 ali9821 commented Jan 20, 2024

List of bugs :
1.Adding chackra-cli in packages.json

  • error :
    package-error
  • solution :
    package-solution
    2.Routing in browser has a bug that on refresh the app lose the route
  • bug : it has problem with
  • solution : replace it with
    3.You have error with imported from @Argent
  • solution : import from react-router-dom

@polarker
Copy link
Member

@ali9821 Thanks for the report. We will check tomorrow. cc @h0ngcha0

h0ngcha0
h0ngcha0 reviewed Jan 22, 2024
View reviewed changes
package.json
@@ -14,7 +14,8 @@
"patch-package": "^6.4.7",
"prettier": ">=2",
"prettier-plugin-import-sort": "^0.0.7",
"ts-node": "^10.8.1"
"ts-node": "^10.8.1",
"@chakra-ui/cli": "^latest"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! 👍
Can we set an explicit version here instead of latest?

@ali9821
Copy link
Author

ali9821 commented Jan 22, 2024 via email

@h0ngcha0
Copy link
Member

h0ngcha0 commented Jan 23, 2024 •
edited
Loading

Yes sure I tested with version 2.4.0 and it was more compatible than 2.4.1 Also look at the other fixes I sent in the description they're important too

Thanks!

h0ngcha0
h0ngcha0 reviewed Jan 23, 2024
View reviewed changes
packages/extension/src/ui/AppRoutes.tsx
import { chakra } from "@chakra-ui/react"
import { FC, ReactNode, isValidElement, useMemo } from "react"
// import { Outlet, Route, Routes } from "react-router-dom" // reinstate in case of issues with @argent/stack-router
import { Outlet, useLocation } from "react-router-dom"
import {Routes} from "react-router-dom" // import Routes from react-router-dom
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have error with imported from @Argent

What kind of error did you get with importing Routes from "@argent/stack-router"?

h0ngcha0
h0ngcha0 reviewed Jan 23, 2024
View reviewed changes
packages/extension/src/ui/index.tsx
@@ -33,8 +33,8 @@ const root = createRoot(container)

root.render(
<StrictMode>
<BrowserRouter>
<HashRouter>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Routing in browser has a bug that on refresh the app lose the route

Could you please give me a bit more info about the issue? Do you mean refreshing the extension wallet?

@ali9821
Copy link
Author

ali9821 commented Jan 24, 2024 via email

@ali9821
Copy link
Author

ali9821 commented Jan 24, 2024 via email

@h0ngcha0
Copy link
Member

When you extend the extension in the browser when You refresh the page, the program can't get the account/tokens route and user get lost you can try with extending wallet in browser and refreshing the page
…
On Tue, Jan 23, 2024, 14:32 Hongchao Liu @.> wrote: @.* commented on this pull request. ------------------------------ In packages/extension/src/ui/index.tsx <#179 (comment)> : > @@ -33,8 +33,8 @@ const root = createRoot(container) root.render( - + Routing in browser has a bug that on refresh the app lose the route Could you please give me a bit more info about the issue? Do you mean refreshing the extension wallet? — Reply to this email directly, view it on GitHub <#179 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AT2YFTNFSJSY577Z6YJQLALYP6KFPAVCNFSM6AAAAABCDE346SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMZYGQ3TONJRGA . You are receiving this because you were mentioned.Message ID: @.***>

Tested it, nice fix 👍

@h0ngcha0
Copy link
Member

You get an error in the code And I remember some time ago when you were changing the route and was getting redirected to some pages the components didn't load and the page was blank
…
On Tue, Jan 23, 2024, 14:28 Hongchao Liu @.> wrote: @.* commented on this pull request. ------------------------------ In packages/extension/src/ui/AppRoutes.tsx <#179 (comment)> : > import { chakra } from @./react" import { FC, ReactNode, isValidElement, useMemo } from "react" // import { Outlet, Route, Routes } from "react-router-dom" // reinstate in case of issues with @argent/stack-router import { Outlet, useLocation } from "react-router-dom" +import {Routes} from "react-router-dom" // import Routes from react-router-dom You have error with imported from @Argent https://github.com/Argent What kind of error did you get with importing Routes from @./stack-router"? — Reply to this email directly, view it on GitHub <#179 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AT2YFTMNUID26T23FG4PHK3YP6JVVAVCNFSM6AAAAABCDE346SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMZYGQ3DSNJZGI . You are receiving this because you were mentioned.Message ID: @.***>

How do I reproduce this error in the code?
The import works ok for me so I wanna figure out a way to verify your fix.

Steps i use to build:

yarn
yarn run build

@ali9821
Copy link
Author

ali9821 commented Jan 25, 2024 via email

@ali9821
Copy link
Author

ali9821 commented Jan 25, 2024 via email

@ali9821
Copy link
Author

ali9821 commented Jan 25, 2024 via email

@h0ngcha0
Copy link
Member

BTW do you need any Web3 Frontend dev in your team?
…
On Thu, Jan 25, 2024, 15:29 Ali Azarian @.> wrote: You're welcome On Thu, Jan 25, 2024, 15:04 Hongchao Liu @.> wrote: > When you extend the extension in the browser when You refresh the page, > the program can't get the account/tokens route and user get lost you can > try with extending wallet in browser and refreshing the page > … <#m_591274396940063359_m_2036684164093062916_> > On Tue, Jan 23, 2024, 14:32 Hongchao Liu @.> wrote: @.** commented on > this pull request. ------------------------------ In > packages/extension/src/ui/index.tsx <#179 (comment) > <#179 (comment)>> > : > @@ -33,8 +33,8 @@ const root = createRoot(container) root.render( - + > Routing in browser has a bug that on refresh the app lose the route Could > you please give me a bit more info about the issue? Do you mean refreshing > the extension wallet? — Reply to this email directly, view it on GitHub <#179 > (review) > <#179 (review)>>, > or unsubscribe > https://github.com/notifications/unsubscribe-auth/AT2YFTNFSJSY577Z6YJQLALYP6KFPAVCNFSM6AAAAABCDE346SVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQMZYGQ3TONJRGA > . You are receiving this because you were mentioned.Message ID: @.> > > Tested it, nice fix 👍 > > — > Reply to this email directly, view it on GitHub > <#179 (comment)>, > or unsubscribe > https://github.com/notifications/unsubscribe-auth/AT2YFTJ5UPZUVUQ7Q4JSGOTYQI7LPAVCNFSM6AAAAABCDE346SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBZHE4DMMBRG4 > . > You are receiving this because you were mentioned.Message ID: > @.*> >

AFAIK, not at the moment, might change in the future. We do have a grant program in case you are interested.

You are welcome to join our discord channel, many dev related topics are discussed there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h0ngcha0 h0ngcha0 h0ngcha0 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ali9821 @polarker @h0ngcha0