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.

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

PoC: Sidepanel #596

Closed
wants to merge 17 commits into from
Closed

PoC: Sidepanel #596

wants to merge 17 commits into from

Conversation

everdimension
Copy link
Member

@everdimension everdimension commented Aug 21, 2024

  • When sidepanel is open, react to updated tab origin and to "chain changed" event
  • Adapt layout for sidepanel
  • What if dapp request comes NOT from the current (active) tab? Handle this case

@everdimension everdimension force-pushed the poc/sidepanel-api-wip branch 2 times, most recently from 2d3e200 to 7f7da8b Compare August 23, 2024 17:14
Copy link

github-actions bot commented Aug 26, 2024

📦 build.zip [updated at Sep 3, 7:54:32 PM UTC]

@zerts
Copy link
Contributor

zerts commented Sep 3, 2024

In Arc if you choose "Prefer Sidepanel", extension goes into broken state. Cause sidepanel is not supported, so nothing opens.

@zerts
Copy link
Contributor

zerts commented Sep 3, 2024

If you prefer sidebar and sign/send/connect screen opens, you lose context and route where you were before in extension.

@zerts
Copy link
Contributor

zerts commented Sep 3, 2024

If I prefer sidebar, close it and try to sign transaction, sidebar doesn't open on this request -> I need to do it manually.

import { emitter } from '../events';

export function navigateProgrammatically(params: { pathname: string }) {
emitter.emit('navigationRequest', params);
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this wrapper over navigate?

Copy link
Member Author

Choose a reason for hiding this comment

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

because navigate works only in context of react router
we should migrate to creating a router instance later: https://reactrouter.com/en/main/upgrading/v6-data

navigationRequest: (params: { pathname: string }) => void;
// this event means that some dapp-related data is updated (e.g. current account, chain)
ethereumEvent: () => void;
'sidepanel/activeTabUpdated': () => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this event be called just activeTabUpdated?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know :)

@zerts
Copy link
Contributor

zerts commented Sep 3, 2024

After choosing "Open sidepanel once" and closing it, dapp requests don't open popup

@@ -0,0 +1,3 @@
export function isSidepanelSupported() {
return globalThis.chrome && 'sidePanel' in chrome;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, interesting, why this didn't prevent sidebar controls from appearing in Arc?

Copy link
Member Author

@everdimension everdimension Sep 3, 2024

Choose a reason for hiding this comment

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

Yeah, looks like a bug in Arc

// it's ok, it might've not been open at all
}
const url = getSidepanelUrl();
if (!currentWindowId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we move this condition to the beginning of this function? To avoid closing sidepanel without opening it next

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, I'm thinking that if user pressed a shortcut and sidepanel is opened and it's possible to close it, it should be closed. That way the user interaction won't seem broken

@everdimension
Copy link
Member Author

Closed in favor of #617

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants