-
Notifications
You must be signed in to change notification settings - Fork 24
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
Feature/sidepanel support and flow [WLT-5590] #617
Conversation
📦 build.zip [updated at Oct 28, 3:42:29 PM UTC] |
5fe3af4
to
1cb262e
Compare
@@ -39,6 +39,7 @@ body { | |||
*/ | |||
display: grid; | |||
--body-width: 425px; | |||
--body-max-width: 800px; |
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.
should we add this to popup mode too?
also login page layout is a bit broken on wide screen
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.
Popup can't be this wide
@@ -42,18 +46,33 @@ export const sessionCacheService = new PortMessageChannel({ | |||
}) as RPCPort<SessionCacheService>; | |||
|
|||
class WindowPort extends PortMessageChannel { | |||
static maybeRestoreRouteForSidepanel() { | |||
if (urlContext.windowType === 'sidepanel') { | |||
// TODO: navigate to location that user was on before opening the request view? |
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.
Should we implement this before release?
src/background/Wallet/Wallet.ts
Outdated
@@ -1631,6 +1639,10 @@ class PublicController { | |||
origin: string, | |||
props: NotificationWindowProps<T> | |||
) { | |||
if (debugValue && process.env.NODE_ENV === 'development') { |
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.
Do we need to keep this?
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.
Nope
src/shared/UrlContext.ts
Outdated
windowType: | ||
(params.get(UrlContextParam.windowType) as WindowType) || 'popup', | ||
windowType: getWindowType(params), | ||
// (params.get(UrlContextParam.windowType) as WindowType) || 'popup', |
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.
delete?
@@ -6,12 +6,19 @@ import type { | |||
} from './types/UrlContext'; | |||
import { UrlContextParam } from './types/UrlContext'; | |||
|
|||
function getWindowType(params: URLSearchParams): WindowType { | |||
if (window.location.pathname.startsWith('/sidepanel')) { |
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.
Do we use this case somewhere?
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, we use urlContext.windowType
in a lot of places :)
@@ -715,7 +715,10 @@ function SendTransactionContent({ | |||
const windowId = params.get('windowId'); | |||
invariant(windowId, 'windowId get-parameter is required'); | |||
windowPort.reject(windowId); | |||
navigate(-1); | |||
// navigate(-1); |
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.
remove?
src/shared/sidepanel/types.ts
Outdated
@@ -0,0 +1,21 @@ | |||
import { isObj } from '../isObj'; | |||
|
|||
interface IsOpenRequest { |
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.
IsSidepanelOpenRequest
?
export function isSidepanelMessageRequest( | ||
x: unknown | ||
): x is SidepanelMessageRequest { | ||
return isObj(x) && isObj(x.payload) && 'method' in x.payload; |
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.
should we check here that x.payload.method is one of options 'is-sidepanel-open'/'close-sidepanel'?
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.
This would make it harder to add more methods
Good case for zod
btw
@@ -0,0 +1,3 @@ | |||
export function isSidepanelSupported() { | |||
return globalThis.chrome && 'sidePanel' in chrome; |
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.
&& chrome.sidePanel != null
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.
This is a canonical way to check for browser apis
There's no reason for the property to exist and be nullish, I think. And we're not checking deeper anyway
export async function openSidePanel({ | ||
pathname, | ||
searchParams, | ||
openOptions, |
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.
Do we need this param?
// const id = nanoid(); | ||
// searchParams.append('windowId', String(id)); | ||
// We use HashRouter currently | ||
if (searchParams) { |
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.
As I see there is only one case of this function call and searchParams are always null
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.
This is still a reasonable api I think
e948e7f
to
c27f582
Compare
React to active tab change, URL change and dapp chain change
Refactor NotificationWindow to handle sidepanel and edge cases such as multiple windows opened where some have sidepanel opened and some don't
c27f582
to
92d5d7c
Compare
No description provided.