-
Notifications
You must be signed in to change notification settings - Fork 100
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.
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
fix: debugger initial frame load + frame ui image loading state on change #279
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
packages/debugger/app/page.tsx
Outdated
url || process.env.NEXT_PUBLIC_HOST || "http://localhost:3000" | ||
); | ||
const url = useMemo(() => { | ||
const fallbackURL = process.env.NEXT_PUBLIC_HOST || 'http://localhost:3000'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually a client side exception occurs when a server isn't running - does this handle that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this just solves issues with manipulating the URL and also loading the URL on initial load. Not sure what you mean by exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean this @stephancill ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then we should probably not fill the URL input with default URL as well right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's useful to pre-populate the input, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok so if the url query is filled, we should load it automatically. If there is no url query param, then we should just show topbar, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
lgtm! |
Change Summary
This PR fixes:
This PR also removes unnecessary url input state, etc.
Merge Checklist