Serve a page on / for payjoin-directory#824
Conversation
This commit addresses payjoin#818
|
|
||
| let html = r#" | ||
| <!DOCTYPE html> | ||
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8"> | ||
| <title>Payjoin Directory</title> | ||
| <style> | ||
| body { | ||
| background-color: #0f0f0f; | ||
| color: #eaeaea; | ||
| font-family: 'Courier New', Courier, monospace; | ||
| padding: 2rem; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| height: 100vh; | ||
| } | ||
| .container { | ||
| background: #1a1a1a; | ||
| border: 1px solid #333; | ||
| border-radius: 8px; | ||
| padding: 2rem; | ||
| box-shadow: 0 0 10px rgba(0, 170, 255, 0.2); | ||
| text-align: center; | ||
| } | ||
| h1 { | ||
| color: #00aaff; | ||
| margin-bottom: 1rem; | ||
| } | ||
| p { | ||
| color: #ccc; |
There was a problem hiding this comment.
mixing html with code is bad practice , However since this is just a minimal page i think those rules can be relaxed here
There was a problem hiding this comment.
agreed, the directory is not going to have any kind of frontend that doesn't make much sense long term either so IMO it's a good idea to keep it simple, this is a bad practice is more for web development in general but doesn't apply here
Pull Request Test Coverage Report for Build 16002894193Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
nothingmuch
left a comment
nothingmuch
left a comment
There was a problem hiding this comment.
it looks nice and clean, exactly what i was hoping for
i have some suggestions but I don't feel i'm the right person to make a call about...
this should probably use the color theme on pdk and payjoin sites, so pink instead of blue, @thebrandonlucas wdyt?
i also like the look of the monospace font but also not sure that's very consistent with existing style
payjoin-directory/src/lib.rs
Outdated
| <h1>Payjoin Directory</h1> | ||
| <p>this is a directory used to facilitate asynchronous payments between supporting Bitcoin wallets.</p> |
There was a problem hiding this comment.
this should link to payjoin.org or https://payjoin.org/docs/how-it-works/payjoin-v2-bip-77
maybe also explain briefly that the directory is a mailbox service? idk...
|
Here's a screenshot to save time for other reviewers
|
|
updated look for reviewers |
|
utACK. Simple and straightforward. @nothingmuch Frankly I don't think we should worry too much about the design of this other than maybe consistent color scheme (yet) as we'd really like to reevaluate and overhaul our branding at some point once that becomes a greater priority. What we have now was really just an initial attempt that never got a more serious, dedicated effort. In which case whatever we put here would then change as well. |
payjoin-directory/src/lib.rs
Outdated
| <body> | ||
| <div class="container"> | ||
| <h1>Payjoin Directory</h1> | ||
| <p>This is a mailbox for payjoin transactions, which is used to facilitate asynchronous payments between supporting Bitcoin wallets.</p> |
There was a problem hiding this comment.
| <p>This is a mailbox for payjoin transactions, which is used to facilitate asynchronous payments between supporting Bitcoin wallets.</p> | |
| <p>The Payjoin Directory provides a rendezvous point for sender and receiver to meet. The directory stores Payjoin payloads to support asynchronous communication.</p> |
Facilitate payments is pretty loaded language I'm not the most comfortable with. I'd like to borrow from the BIP with as plain language as is possible https://github.com/DanGould/bips/blob/pjv2-bak/bip-0077.mediawiki#directory-interactions
The directory has mailboxes, it isn't itself a mailbox according to BIP 77 as well.
There was a problem hiding this comment.
"facilitate communication" is not only less loaded, it's also more accurate FWIW
|
spacebear21
left a comment
spacebear21
left a comment
There was a problem hiding this comment.
I agree the theme could be overhauled but this can be a follow-up for when we produce better design guidelines.
|
@spacebear21 I imagine you considered #840 before merging this, but I want to bring it up for others to see. we're in a precarious spot adding new code while 0.24.0 has been released before all of the others, but I know this change just changes the index response and won't cause a problem. |
6 participants
This commit addresses #818 .