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

Subject line #8

Merged
merged 6 commits into from
May 3, 2019
Merged

Subject line #8

merged 6 commits into from
May 3, 2019

Conversation

codi45
Copy link
Contributor

@codi45 codi45 commented Apr 28, 2019

Add Subject line

@ldmberman
Copy link
Member

@codi45 thank you for opening a PR! I've tried it out and have some feedback.

Sending messages does not seem to work with this update. I get TypeError: document.getElementById(...) is null (I deployed this version here).

Also, the transaction ID is still displayed on top of the message page while the issue description suggests it should be replaced by the subject line, which in turn should be the TX ID for now.

@codi45
Copy link
Contributor Author

codi45 commented May 2, 2019

Ok fixed Id missing and corrected subjectLine value to TX ID ;

@ldmberman
Copy link
Member

Thank you for the update @codi45! I like how it looks and works now.

I have only two requests.

  1. Could you please remove the commented out lines and format the code using our configuration for ESLint? It's very easy:
npm install
npx eslint --fix .

Ignore all the warnings it displays, just commit the changes it makes.

  1. Since you have added a way to specify a subject line, it would be great to display it instead of TX ID when present. Should be a simple addition to the code.

Thank you!

@codi45
Copy link
Contributor Author

codi45 commented May 3, 2019

Ok eslint --fix and subject line content display when available otherwise TX ID

@ldmberman
Copy link
Member

@codi45 looks good, thanks!

@ldmberman ldmberman merged commit 7633024 into ArweaveTeam:master May 3, 2019
@codi45 codi45 deleted the subjectLine branch May 3, 2019 18: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.

2 participants