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

AddNotes: Add notes after payment #1447

Merged
merged 11 commits into from May 30, 2023
Merged

Conversation

shubhamkmr04
Copy link
Contributor

@shubhamkmr04 shubhamkmr04 commented Apr 25, 2023

This PR adds functionality to add notes with lightning payments, Onchain Transactions and Invoice, and later, users can see the notes inside the Activity view and edit them from there.

Screenshot_20230426-193710
Screenshot_20230426-193944
Screenshot_20230426-194058
Screenshot_20230426-194133
Screenshot_20230426-201451

@shubhamkmr04 shubhamkmr04 marked this pull request as ready for review April 26, 2023 12:54
<Button
onPress={async () => {
navigation.navigate('Wallet');
const key: any = payment_hash || txid;
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 make the key prefixed with note-

}}
containerStyle={{ position: 'absolute', bottom: 40 }}
buttonStyle={{ padding: 15 }}
title="add note"
Copy link
Contributor

Choose a reason for hiding this comment

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

use a locale for this string too

@@ -35,10 +39,18 @@ export default class PaymentView extends React.Component<PaymentProps> {
if (lnurlpaytx) {
this.setState({ lnurlpaytx });
}
EncryptedStorage.getItem(payment.payment_hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure you update the key format here too

@kaloudis
Copy link
Contributor

Looking solid overall. Please add some screenshots to the PR.

@shubhamkmr04
Copy link
Contributor Author

Looking solid overall. Please add some screenshots to the PR.

Thanks! Did the changes.

@kaloudis kaloudis added this to the v0.7.6 milestone Apr 26, 2023
@kaloudis
Copy link
Contributor

I think we should also add this functionality to Invoices (LN payments you've requested) as well. Yes you have some information from the memo but it would be nice to add notes you didn't expose to the payer.

@kaloudis
Copy link
Contributor

Also, please add a screenshot of the Add Note view

@shubhamkmr04
Copy link
Contributor Author

I think we should also add this functionality to Invoices (LN payments you've requested) as well. Yes you have some information from the memo but it would be nice to add notes you didn't expose to the payer.

Okay. I'll do it

locales/en.json Outdated
@@ -556,6 +559,7 @@
"views.Transaction.numConf": "Number of Confirmations",
"views.Transaction.status": "Status",
"views.Transaction.timestamp": "Timestamp",
"views.Transaction.notes": "Notes",
Copy link
Contributor

Choose a reason for hiding this comment

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

wee just just use a singular definition here instead of requiring translators to translate it multiple times.

Also think it might be better as Note singular

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I thought there are different views in which we are using them so I did that, like we would have to use {localeString('views.Payment.notes')} in Transaction view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its okay, I'll use only one definition

}}
multiline
numberOfLines={0}
style={{ fontSize: 20 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

should also have color: themeColor('text')

<Button
title={localeString('views.AddNotes.AddNote')}
onPress={async () => {
navigation.navigate('Wallet');
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of navigating back to the wallet it should navigate back to the previous screen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh I was also thinking to do it, will do it

<Header
leftComponent="Back"
centerComponent={{
text: localeString('views.AddNotes.AddNote'),
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this says 'Update Note' if the note already exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

@kaloudis
Copy link
Contributor

Lastly, I think you should also be able to go to a AddNote view if you click on the note in the Payment/Invoice/TX view

@ZeusLN ZeusLN deleted a comment from shubhamkmr04 Apr 26, 2023
@ZeusLN ZeusLN deleted a comment from shubhamkmr04 Apr 26, 2023
@shubhamkmr04
Copy link
Contributor Author

shubhamkmr04 commented Apr 26, 2023

Lastly, I think you should also be able to go to a AddNote view if you click on the note in the Payment/Invoice/TX view

Like when I tap on the Note itself?

@kaloudis
Copy link
Contributor

Lastly, I think you should also be able to go to a AddNote view if you click on the note in the Payment/Invoice/TX view

Like when I tap on the Note itself?

exactly

const key: any =
'note-' + (payment_hash || txid || RPreimage);
await EncryptedStorage.setItem(key, notes);
if (!noteKeys.includes(key)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be populated when you load the note, or perhaps in a a separate note store.

You also need to add logic to remove it from the index if the user deletes the note

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 have added the logic to remove keys from noteKeys array, in onChangeText prop

@observable public noteKeys: string[] = [];

@action
public storingNoteKeys = async (key: string, notes: string) => {
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 call it storeNoteKeys or saveNoteKeys

onChangeText={(text: string) => {
this.setState({ notes: text });
if (!text) {
const key: any =
Copy link
Contributor

@kaloudis kaloudis May 15, 2023

Choose a reason for hiding this comment

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

type is string here

@kaloudis
Copy link
Contributor

Add a Note button should always be at the bottom:
Simulator Screen Shot - iPhone 14 Pro - 2023-05-22 at 12 52 11

<Header
leftComponent="Back"
centerComponent={{
text: notes
Copy link
Contributor

Choose a reason for hiding this comment

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

this text shouldn't be dependent on whether the note in the state is set, rather if a previous note has been saved

Copy link
Contributor

Choose a reason for hiding this comment

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

same with the button at the bottom of this view

@kaloudis
Copy link
Contributor

Note button styling is inconsistent on the Traction view
Simulator Screen Shot - iPhone 14 Pro - 2023-05-25 at 14 08 07
Simulator Screen Shot - iPhone 14 Pro - 2023-05-25 at 14 07 58

@kaloudis kaloudis merged commit 3666e78 into ZeusLN:master May 30, 2023
3 checks passed
@AndySchroder
Copy link

Are these notes stored on LND or on the phone?

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.

None yet

3 participants