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

Fix Popup based on Figma files #1039

Merged
merged 16 commits into from
May 19, 2022
Merged

Fix Popup based on Figma files #1039

merged 16 commits into from
May 19, 2022

Conversation

wirednkod
Copy link
Contributor

@wirednkod wirednkod commented May 12, 2022

Fixes #977

image

image

@wirednkod wirednkod changed the title Make Accordion as separate component from NetworkTabs Fix Popup based on Figma files May 12, 2022
@wirednkod
Copy link
Contributor Author

wirednkod commented May 12, 2022

There are still some things missing - but has to do with the way that smoldot API is implemented.
e.g. The integrated chains do not report peers and/or sync status at the moment (see here).
I understand that this will not be fixed - thus we cannot show the pop up (syncing/peers) when extension opens without any tab requesting connection to chains.

Another non-implemented part of the pop-up is the Ban App dropdown that - as discussed before - probably will not be implemented.

Once this PR is merged - the user will be able to either disconnect full app of a multi-network dApp or just 1 of the chains from the many connected in same tab/port - which I really like and Im glad this is now done.

@tomaka
Copy link
Contributor

tomaka commented May 16, 2022

I understand that this #855 - thus we cannot show the pop up (syncing/peers) when extension opens without any tab requesting connection to chains.

I can fix this, just maybe not with the solution in #855
We shouldn't change our design/go through intermediary steps just to bypass bugs

@wirednkod wirednkod requested a review from tomaka May 18, 2022 11:51
<div className="icon w-7">
{knownChains.includes(icon) ? icon : "?"}
</div>
<div className="pl-2">{network}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super confident in this. This value is untrusted and entirely decided by the website. A website could have a name that is super long or has tons of weird unicode characters.

Copy link
Contributor Author

@wirednkod wirednkod May 19, 2022

Choose a reason for hiding this comment

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

Im not sure I understand - this part of code is showing the network name. We don't show the app's name. So even if there is a custom chain/parachain that is named: 123AS$@#$SomethingMoreAnd123;'_'ThisCanGoOnUntilItIsVeryLong, still will just appear as the "network name" and icon as ?. I'm not sure what the vulnerability is.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no "vulnerability", just someone could completely destroy your CSS with a very long name, unless you made sure that it can't happen

@wirednkod wirednkod requested a review from tomaka May 19, 2022 08:14
@wirednkod wirednkod merged commit 116747a into paritytech:main May 19, 2022
@wirednkod wirednkod deleted the nik-extension-popup-based-onf-figma branch May 19, 2022 09:10
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.

Refactor Popup Page UI to to adapt to figma files
2 participants