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

refactor(home): extract AllowanceView and DefaultView #1613

Merged
merged 9 commits into from
Oct 25, 2022
Merged

Conversation

lisabaut
Copy link
Contributor

@lisabaut lisabaut commented Oct 13, 2022

Describe the changes you have made in this PR

This is a bigger refactoring which divides the Home screen (initial screen in PopUp) into two views:

  • AllowanceView
  • DefaultView

The refactoring required these steps:

  • align all places where the payments for the component TransactionTable are created
  • => this fixes this ticket too Refactor: use virtual attribute on payment for title #1356 (correct @escapedcat ?)
  • extend Transaction type with publisherLink as this is the main difference between the TransactionTable data in views for Allowance, Default (no Allowance), PublisherSingleView
  • extracting components in two new folders screens/Home/DefaultView and screens/Home/AllowanceView

⭐ PLEASE follow the commit history if you have problems reading the diff ⭐


Link this PR to an issue [optional]

Closes #1359
Closes #1356

Type of change

  • feat: New feature (non-breaking change which adds functionality)

How has this been tested?

  • On stacker news with setting a budget, editing it, deleting it
  • adding example metatag to a website and check keysend (<meta name="lightning" content="method=keysend; address=030a58b8653d32b99200a2334cfe913e51dc7d155aa0116c176657a4f1722677a3; customKey=696969; customValue=3wQCCrfOAMYNzOh1sL05" />)

Would be cool to have a list of sites/pages to test in general.
And I can add more tests to this PR too of course.

Checklist

  • My code follows the style guidelines of this project and performed a self-review of my own code
  • New and existing tests pass locally with my changes
  • I checked if I need to make corresponding changes to the documentation (and made those changes if needed)

@lisabaut lisabaut requested review from escapedcat and bumi October 13, 2022 13:50
@github-actions
Copy link

github-actions bot commented Oct 13, 2022

🚀 Thanks for the pull request!

Here are the current build files for testing:

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.


This build is brought to you by: Adam Fiscor (who recently dropped 1337 sats):

The future is bright. We just have a lot of work to do.

Want to sponsor the next build? send some sats to ⚡️[email protected] (don't forget to provide your name)

Don't forget: keep earning sats!

@bumi bumi requested a review from reneaaron October 13, 2022 14:24
@escapedcat escapedcat requested a review from im-adithya October 17, 2022 07:09
src/app/screens/Home/AllowanceView/index.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

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

---package-lock.json must not be in the repo. we use yarn.---
github navigation fu*up sorry.

@lisabaut lisabaut requested a review from bumi October 24, 2022 10:00
Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

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

With this change we no longer show the tipping option if we have an allowance.

The "send satoshi" option will only be available for sites without allowance.
Once a user clicks on a lightning: link on a page there will be an allowances saved.
So I think this restriction is too much?

@reneaaron what have been thoughts and cases here?

src/app/components/TransactionsTable/index.tsx Outdated Show resolved Hide resolved
src/app/screens/Home/index.tsx Show resolved Hide resolved
src/app/screens/Home/index.tsx Show resolved Hide resolved
@lisabaut lisabaut requested a review from bumi October 25, 2022 08:27
Copy link
Contributor

@reneaaron reneaaron left a comment

Choose a reason for hiding this comment

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

Reviewed the code and tested the extension.

@bumi I can't really imagine the effects of our scripts using WebLN inpage (like with the Youtube buttons). But let's take care of that as we get there.

src/app/components/TransactionsTable/index.tsx Outdated Show resolved Hide resolved
@bumi
Copy link
Collaborator

bumi commented Oct 25, 2022

@bumi I can't really imagine the effects of our scripts using WebLN inpage (like with the Youtube buttons). But let's take care of that as we get there

Yes, we'll see. then it should be pure inpage webln and the page will have an allowance.

@lisabaut
Copy link
Contributor Author

@bumi Please approve this PR, otherwise I can not merge because of the "requested changes"

@escapedcat
Copy link
Contributor

Looks like we're good here. I'll merge once this is 💚

@escapedcat escapedcat merged commit a13b7ee into master Oct 25, 2022
@escapedcat escapedcat deleted the refactor-home branch October 25, 2022 14:46
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.

Home: split into "Allowance" and "Default" views Refactor: use virtual attribute on payment for title
4 participants