Skip to content
This repository has been archived by the owner on Nov 5, 2023. It is now read-only.

Send eth #253

Merged
merged 12 commits into from
Jul 9, 2022
Merged

Send eth #253

merged 12 commits into from
Jul 9, 2022

Conversation

voltrevo
Copy link
Collaborator

@voltrevo voltrevo commented Jul 7, 2022

What is this PR doing?

Enables sending ETH

Demo video

How can these changes be manually tested?

Replicate behavior shown in video.

Does this PR resolve or contribute to any issues?

Resolves #230.

Checklist

  • I have manually tested these changes
  • Post a link to the PR in the group chat

Guidelines

  • If your PR is not ready, mark it as a draft
  • The resolve conversation button is for reviewers, not authors
    • (But add a 'done' comment or similar)

@github-actions github-actions bot added the extension Browser extension related label Jul 7, 2022
Copy link
Collaborator

@jacque006 jacque006 left a comment

Choose a reason for hiding this comment

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

Although we use the $ symbol to prevent shadowing in certain cell calls, I'm against its proliferation into other variable names such as here and elsewhere in this PR. $ is used commonly for templating and in frameworks like Angular, and is not a common TS/JS variable symbol that I have run across. Adds confusion when reading, especially when $, s, and S are in close proximity.

If there are two vars with similar names, I would just be more specific/verbose with naming one of them. This could also be a good case for enabling a lint rule around variable naming, but don't want to have to disable on every line with $ to prevent shadowing :)

@voltrevo
Copy link
Collaborator Author

voltrevo commented Jul 8, 2022

Although we use the $ symbol to prevent shadowing in certain cell calls, I'm against its proliferation into other variable names such as here and elsewhere in this PR. $ is used commonly for templating and in frameworks like Angular, and is not a common TS/JS variable symbol that I have run across. Adds confusion when reading, especially when $, s, and S are in close proximity.

If there are two vars with similar names, I would just be more specific/verbose with naming one of them. This could also be a good case for enabling a lint rule around variable naming, but don't want to have to disable on every line with $ to prevent shadowing :)

Hmm fair enough. I was still figuring out how I felt about it too. Will fix.

@voltrevo
Copy link
Collaborator Author

voltrevo commented Jul 8, 2022

Although we use the $ symbol to prevent shadowing in certain cell calls, I'm against its proliferation...

Fixed

@jacque006 jacque006 merged commit b8ae6e9 into main Jul 9, 2022
@jacque006 jacque006 deleted the bw-230-send-eth branch July 9, 2022 19:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extension Browser extension related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable sending ETH using button
2 participants