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

Add static subcape link to the wallet #3670

Merged
merged 5 commits into from
Nov 18, 2023
Merged

Add static subcape link to the wallet #3670

merged 5 commits into from
Nov 18, 2023

Conversation

jagodarybacka
Copy link
Contributor

@jagodarybacka jagodarybacka commented Nov 14, 2023

Resolves #3669

What

Subscape link in the wallet:

  • hidden on the right
  • visible on hover
  • visible for everyone, no matter the TAHO/veTAHO balance

Button will be hidden later after the beta season will end (#3673)

Testing

  • make sure link is only overlapping the wallet page UI, slideups (accounts list, networks list) should be displayed over the link
  • make sure you can open the Subscape using the link
  • make sure you can use wallet page UI - no regressions
Screen.Recording.2023-11-14.at.15.16.16.mov

Latest build: extension-builds-3670 (as of Sat, 18 Nov 2023 21:35:42 GMT).

@jagodarybacka jagodarybacka self-assigned this Nov 14, 2023
@jagodarybacka jagodarybacka marked this pull request as ready for review November 15, 2023 12:31
ui/public/index.css Outdated Show resolved Hide resolved
.subscape_link {
cursor: pointer;
position: absolute;
z-index: 998; // Above the UI, below the menu
Copy link
Contributor

@xpaczka xpaczka Nov 15, 2023

Choose a reason for hiding this comment

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

Maybe we could set z-index as a CSS variable (not sure how reusable it'd be, up to you)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love to but this is a bigger refactor because we have a lot of z-indexes to take care of, let's do it later

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's create an issue for it then (if it is not created already of course)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hadn't seen variables used for z-indices before, I like that!

Co-authored-by: Michał Paczyński <michal.paka@op.pl>
xpaczka
xpaczka previously approved these changes Nov 15, 2023
Copy link
Contributor

@xpaczka xpaczka left a comment

Choose a reason for hiding this comment

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

:shipit:

type="button"
className={classNames("subscape_link", { visible: isVisible })}
onClick={() => {
window.open("https://app.taho.xyz/", "_blank")?.focus()
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally, we should avoid passing the arrow function directly as a prop to improve performance, because the arrow function in prop creates a new function each time the component renders. In this case, it is a small issue, but in the future, it's good to create separate const openDapp = () => {} (also using useMemo) and only then pass it as a prop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see but in this repo it is a common practice and I believe that if arrow function is assigned to the low-level DOM node (not other React component) the optimisation it provides is super small so we generally were not concerned about it in the past

Copy link
Contributor

Choose a reason for hiding this comment

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

We can leave it as it is here, because it's small issue, but generally in the future we must pay special attention to this considering the general performance problems of the application.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should def do some digging. For the most part we've found that performance issues have come in places that aren't strictly related to rendering (in particular, they've come from the JSON serialization/deserialization that happens for state sync between background and foreground scripts). Could be that we're compounding that, too.

We've reduced a lot of that, but we still do a full JSON serialization of redux state to local storage every 50ms or so during busy times, which I'm guessing is still at the core of a lot of the pain here.

onClick={() => {
window.open("https://app.taho.xyz/", "_blank")?.focus()
}}
onMouseEnter={() => setIsVisible(true)}
Copy link
Contributor

@ioay ioay Nov 15, 2023

Choose a reason for hiding this comment

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

Also here we can create const showButton/hideButton.

const [isVisible, setIsVisible] = useState(false)

return (
<button
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the state isVisible we should return null or button because I don't see the reason to return the button when it's not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is needed all the time, if isVisible = false then the button is tucked away but the icon is still visible

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this points to a tweak we can make to the naming. isVisible should probably be isCompact, isIconOnly, onlyShowIcon or something similar to indicate that we're not hiding the whole link.

Copy link
Contributor

Choose a reason for hiding this comment

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

Want to be clear that I was open to any name, these were just suggestions. So isFullyVisible (55024fa) and isIconOnly (1de5e10) are both fine.

ioay
ioay previously approved these changes Nov 17, 2023
Copy link
Contributor

@ioay ioay left a comment

Choose a reason for hiding this comment

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

LGTM, we can merge it after changing the name of isFullyVisible to isIconOnly or showIcon which was suggested by @Shadowfiend.

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

Let's :shipit:

Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

(That's still a yes post-merge lol.)

@Shadowfiend Shadowfiend merged commit 895580b into main Nov 18, 2023
5 of 6 checks passed
@Shadowfiend Shadowfiend deleted the subscape-link branch November 18, 2023 21:11
@jagodarybacka jagodarybacka mentioned this pull request Nov 22, 2023
Shadowfiend added a commit that referenced this pull request Nov 23, 2023
## What's Changed
* Check connecting to dapp on different network after disconnecting by
@michalinacienciala in #3654
* v0.51.0 by @Shadowfiend in
#3652
* Fix typos by @xiaolou86 in
#3668
* Alchemic Logs: Small improvements for eth_getLogs by @Shadowfiend in
#3664
* Fix getting currently connected dapp info by @jagodarybacka in
#3671
* Add static subcape link to the wallet by @jagodarybacka in
#3670
* Fix e2e tests by @michalinacienciala in
#3651
* Add Alchemy endpoint for Arbitrum Sepolia by @jagodarybacka in
#3665
* Do notify: Set up base NotificationService by @Shadowfiend in
#3666

## New Contributors
* @xiaolou86 made their first contribution in
#3668

**Full Changelog**:
v0.51.0...v0.52.0

Latest build:
[extension-builds-3678](https://github.com/tahowallet/extension/suites/18415208655/artifacts/1067210815)
(as of Wed, 22 Nov 2023 14:53:11 GMT).
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.

Subscape access in wallet extension
4 participants