DSL connection: Add new default wallet toggle top bar #3448

Shadowfiend · 2023-06-06T05:08:26Z

To do this, we also restructure the top bar and, in the process,
the tab bar's management in CorePage. These give us the space
and tools to set up the new DAppConnection component, which now
contains the default wallet toggle and the current dApp connection's
information.

More changes in here than I would like, but it's mostly moving stuff
around from TopMenu into DAppConnection, ActiveDAppConnection, and
DAppConnectionDefaultToggle. I do want to move a lot of the permission
tomfoolery out of the components and into redux and/or the services, but
that'll be later.

Testing

Toggle the top bar's default wallet setting and make sure it is doing what it's supposed to do! Including turning white when on other wallet and green when on Taho.
Check for fade in and fade out when interacting with the dApp connection info lightning bolt, both on tabs where you are already connected and tabs where you are not.
Make sure the lightning bolt is illuminated when the current tab is connected.
Make sure the dApp connection info is still correct when the current tab is connected.

Latest build: extension-builds-3448 (as of Mon, 12 Jun 2023 01:01:56 GMT).

This was being handled in Popup, but these bars are really a property of the page itself—for example, the transaction signing page has a different top bar and no tab bar, pages like Settings have the tab bar but no top bar, and the wallet has both. While this commit does not move all bar handling into the pages themselves, it does start moving the management of them into the page definitions by having `CorePage` take charge of rendering the tab and top bars (or not) depending on the parameters it is passed.

`onColor` and `offColor` parameters allow modifying the colors that the toggle's bulb takes on when it's turned on as well as when it's turned off. `leftToRight`, defaulting to `true`, indicates that the toggle is off on the left and on when on the right. Passing it as `false` flips the two horizontally.

Mostly this means we get to drop an extraneous div, and simplify styling overall to let flexbox do its thing instead of forcing widths and heights in several places.

The default toggle is always visible now, so showing the banner when a recent change has been made is unnecessary.

This component allows the user to manage the active dApp connection, as well as allowing them to always toggle the default wallet to/from Taho. The popup window with information on connecting to dApps when they are not connected yet needs some refinement for the new default connection approach, but that will be done separately.

These had drifted from the design system at some point, and are now anchored back to the values in Figma. All of the green shades and a few others have been tweaked.

This needs to be paired with a delayed removal of the component from the DOM, which cannot be handled in CSS unfortunately...

This gives all icons that have accessible labels also have proper tooltips for regular users.

hyphenized

Looks good, left some comments. Are we reverting the color changes in this PR or do we want to do that in the other one and merge that here?

ui/pages/Swap.tsx

hyphenized · 2023-06-08T05:56:41Z

ui/components/Shared/SharedToggleButton.tsx

+SharedToggleButton.defaultProps = {
+  leftToRight: true,
+}


Let's move this to default parameters

That's not really what we've been doing... If we want to do that we should systematically convert to it.

To wit:

shadowfiend@theano ~/g/t/extension /cable-connection> ag -l defaultProps | wc -l 20

Ahh yeah there are also a few places where we're doing both. My reasoning is that there's no real advantage to use default props over parameter defaulting and it doesn't play well with typescript ( one known issue with forwarding refs ). Default props is also getting deprecated but, while I don't think it's happening anytime soon I feel this just provides another place where we could miss out potential bugs. If we're going back to defaultProps then I suggest we should create an issue like #1120.

I would love an issue--but to move away from defaultProps. I just don't want to do it inconsistently, I think using the language feature rather than the framework feature is the right call.

For context, IIRC when we first built things out we had issues with typing and linting and param defaults which, combined with how common defaultProps was in the react literature, convinced us to stick to that to start with.

ui/public/index.css

ui/components/DAppConnection/DAppConnection.tsx

ui/components/Shared/SharedToggleButton.tsx

ui/components/DAppConnection/DAppConnectionDefaultToggle.tsx

hyphenized · 2023-06-08T22:53:38Z

ui/components/Core/CorePage.tsx

@@ -36,16 +37,15 @@ export default function CorePage(props: Props): ReactElement {
            align-items: center;
            background-color: var(--hunter-green);
            z-index: 10;
-            height: calc(100vh - ${barSpace}px);
-            margin-top: ${hasTopBar ? "0px" : "-64px"};
+            height: 100%;


This is making pages larger which adds scrolling where it not needed 👀

Yeah, I remember having to disentangle some serious stuff when I tackled this a few months ago. I think this is the least bad option without going deep on the DOM (which I'm planning to do next week), since the bar space situation has changed with the new work.

Are there any places where this seems egregious right now?

ui/components/DAppConnection/DAppConnection.tsx

ui/components/Core/CorePage.tsx

🧹🧹🧹

I personally would love to make everything in CSS kebab-case, but we're deep into using snake_case. Notably, fadeIn is in camelCase; it will be converted to snake_case soon.

This reverts commit 76ec40e. While there has been significant drift between these colors and the same named ones used in Figma, checking in with design indicated that the drift is a Figma issue. Pending a review on the design side of what's in Figma, we're bringing these back to their current values.

Shadowfiend · 2023-06-09T02:41:30Z

Made all of the changes I'm down for rn, but happy to discuss the other stuff further if desired!

ui/public/index.css

Co-authored-by: Jorge Luis <28708889+hyphenized@users.noreply.github.com>

hyphenized

🚢 🚢 🚢

kkosiorowska · 2023-06-13T15:03:48Z

I noticed one small style issue caused by these changes when the feature flag is enabled. I just reported it. #3474

Shadowfiend · 2023-06-13T15:22:35Z

Good catch thank you!

@michalinacienciala

## What's Changed * Fix broken E2E test removing wallet by @michalinacienciala in #3463 * DSL connection: Add new default wallet toggle top bar by @Shadowfiend in #3448 * Pull asset prices from on chain oracles by @0xDaedalus in #3264 * Cable Connection: Implement standard flow for dApp connect popup by @Shadowfiend in #3451 * Slide On Over: Drop an unnecessary z-index on CorePage by @Shadowfiend in #3476 * Restructure the settings page by @kkosiorowska in #3450 * Exclude `article` and `result` types for abilities by @kkosiorowska in #3479 * Release 0.37.0 by @Shadowfiend in #3461 **Full Changelog**: v0.37.0...v0.38.0 Latest build: [extension-builds-3480](https://github.com/tahowallet/extension/suites/13630673975/artifacts/752046785) (as of Thu, 15 Jun 2023 13:51:01 GMT).

Base automatically changed from dial-up-connection to main June 6, 2023 17:03

Shadowfiend force-pushed the dsl-connection branch from 0fffc8f to e777c49 Compare June 6, 2023 18:38

Shadowfiend added 8 commits June 6, 2023 14:41

Adjust TopMenu spacing and structure for new layout

9723c0d

Mostly this means we get to drop an extraneous div, and simplify styling overall to let flexbox do its thing instead of forcing widths and heights in several places.

Hide WalletToggleDefaultBanner in the new dApp connections flow

ba0c3dc

The default toggle is always visible now, so showing the banner when a recent change has been made is unnecessary.

Fix several named CSS color variables

76ec40e

These had drifted from the design system at some point, and are now anchored back to the values in Figma. All of the green shades and a few others have been tweaked.

Add top-level fade-out class for fading components out

c67585f

This needs to be paired with a delayed removal of the component from the DOM, which cannot be handled in CSS unfortunately...

Introduce a quick fade in/out animation for connected dApp info

d45eaf8

Shadowfiend force-pushed the dsl-connection branch from e777c49 to d45eaf8 Compare June 6, 2023 18:42

Shadowfiend marked this pull request as ready for review June 6, 2023 18:46

Shadowfiend added 2 commits June 6, 2023 14:53

i18n for DAppConnectionDefaultToggle component

25d125e

Add title to SharedIcon for aria labels

e49827d

This gives all icons that have accessible labels also have proper tooltips for regular users.

Shadowfiend mentioned this pull request Jun 7, 2023

Cable Connection: Implement standard flow for dApp connect popup #3451

Merged

6 tasks

Shadowfiend requested a review from hyphenized June 8, 2023 00:58

Shadowfiend mentioned this pull request Jun 8, 2023

Repeater Connection: Forward requests to new default when default is switched off during dApp connection flow #3462

Merged

3 tasks

hyphenized suggested changes Jun 8, 2023

View reviewed changes

Shadowfiend added 5 commits June 8, 2023 22:39

Switch an interface for a type alias

b76b732

🧹🧹🧹

Tweak useBackgroundSelector invocation to avoid indirection

75417a5

fade-out class becomes fade_out for consistency

924a516

I personally would love to make everything in CSS kebab-case, but we're deep into using snake_case. Notably, fadeIn is in camelCase; it will be converted to snake_case soon.

Add a TODO for an unimplemented lookup

45a45a3

hyphenized reviewed Jun 9, 2023

View reviewed changes

ui/public/index.css Outdated Show resolved Hide resolved

Fix a crazy typo in fade out duration

6224b76

Co-authored-by: Jorge Luis <28708889+hyphenized@users.noreply.github.com>

hyphenized approved these changes Jun 12, 2023

View reviewed changes

hyphenized enabled auto-merge June 12, 2023 00:59

Merge branch 'main' into dsl-connection

2135d9d

hyphenized merged commit a8504f6 into main Jun 12, 2023

hyphenized deleted the dsl-connection branch June 12, 2023 01:01

kkosiorowska mentioned this pull request Jun 15, 2023

v0.38.0 #3480

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DSL connection: Add new default wallet toggle top bar #3448

DSL connection: Add new default wallet toggle top bar #3448

Shadowfiend commented Jun 6, 2023 •

edited by github-actions bot

Loading

hyphenized left a comment

hyphenized Jun 8, 2023

Shadowfiend Jun 9, 2023

Shadowfiend Jun 9, 2023

hyphenized Jun 9, 2023

Shadowfiend Jun 9, 2023

hyphenized Jun 8, 2023

Shadowfiend Jun 9, 2023

Shadowfiend commented Jun 9, 2023

hyphenized left a comment

kkosiorowska commented Jun 13, 2023

Shadowfiend commented Jun 13, 2023

DSL connection: Add new default wallet toggle top bar #3448

DSL connection: Add new default wallet toggle top bar #3448

Conversation

Shadowfiend commented Jun 6, 2023 • edited by github-actions bot Loading

Testing

hyphenized left a comment

Choose a reason for hiding this comment

hyphenized Jun 8, 2023

Choose a reason for hiding this comment

Shadowfiend Jun 9, 2023

Choose a reason for hiding this comment

Shadowfiend Jun 9, 2023

Choose a reason for hiding this comment

hyphenized Jun 9, 2023

Choose a reason for hiding this comment

Shadowfiend Jun 9, 2023

Choose a reason for hiding this comment

hyphenized Jun 8, 2023

Choose a reason for hiding this comment

Shadowfiend Jun 9, 2023

Choose a reason for hiding this comment

Shadowfiend commented Jun 9, 2023

hyphenized left a comment

Choose a reason for hiding this comment

kkosiorowska commented Jun 13, 2023

Shadowfiend commented Jun 13, 2023

Shadowfiend commented Jun 6, 2023 •

edited by github-actions bot

Loading