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

add Bitcoin Connect #343

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

add Bitcoin Connect #343

wants to merge 7 commits into from

Conversation

Alan-AC7
Copy link

@Alan-AC7 Alan-AC7 commented Nov 27, 2023

Includes button and Modal to connect Lightning wallets. with BitcoinConnect users of Alby, Mutiny or Umbrel wallets can zap on mobile in the PWA.

Here is the repo of the component: https://github.com/getAlby/bitcoin-connect.gi

@Alan-AC7
Copy link
Author

image

Copy link

vercel bot commented Nov 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
flycat-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 8, 2023 6:47pm

@digi-monkey
Copy link
Owner

digi-monkey commented Nov 28, 2023

@Alan-AC7
image

I think the <BitcoinButton /> needs a key prop since it is in an array

@digi-monkey
Copy link
Owner

also, can you introduce what is this Bitcoin Connect trying to do?

@Alan-AC7
Copy link
Author

Alan-AC7 commented Nov 28, 2023

also, can you introduce what is this Bitcoin Connect trying to do?

Includes button and Modal to connect Lightning wallets.

Here is the repo of the component: https://github.com/getAlby/bitcoin-connect.git

@roughpandaz
Copy link
Contributor

@digi-monkey Can we add a githook with husky to compile react before deploy to check for these errors?
Otherwise we can see them lol

@Alan-AC7 image

I think the <BitcoinButton /> needs a key prop since it is in an array

@digi-monkey
Copy link
Owner

also, can you introduce what is this Bitcoin Connect trying to do?

Includes button and Modal to connect Lightning wallets.

Here is the repo of the component: https://github.com/getAlby/bitcoin-connect.git

sounds cool, so this can enable users to zap in their mobile phone?

@digi-monkey
Copy link
Owner

@digi-monkey Can we add a githook with husky to compile react before deploy to check for these errors? Otherwise we can see them lol

@Alan-AC7 image
I think the <BitcoinButton /> needs a key prop since it is in an array

yea I think we need a CI that can run for every PR. I am not sure if we should do it in the .husky like every pre-commit we should run yarn build, that might be inconvinent. I am thinking something like github action that can be trigger for every PRs


<List.Item
actions={[

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
key={'btc-connect'}

this should fixed the compiled error

@MoritzKa
Copy link

Yes, with BitcoinConnect users of Alby, Mutiny or Umbrel wallets can zap on mobile in the PWA.

@Alan-AC7
Copy link
Author

2023-11-29-15-48-16_eC16on8h.mp4

package.json Outdated
@@ -112,7 +113,7 @@
},
"scripts": {
"wasm:profile": "cd wasm && rustc --crate-type cdylib --target wasm32-unknown-unknown -C lto -C opt-level=z -o profile.wasm examples/profile.rs",
"dev": "REACT_APP_COMMIT_HASH=$(git rev-parse --short HEAD) next dev",
"dev": "set \"REACT_APP_COMMIT_HASH=$(git rev-parse --short HEAD)\" && next dev",
Copy link

Choose a reason for hiding this comment

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

This should be undone, right?

const BitcoinButton: React.FC<BitcoinButtonProps> = ({ isDisabled }) => {
const handleConnect = () => {
if (!isDisabled) {
alert('Connected!');
Copy link

Choose a reason for hiding this comment

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

I don't think an alert it is needed

@@ -14,6 +14,7 @@ import { joyIdConfig } from 'core/joyid/config';
import Head from 'next/head';
import theme from 'constants/theme';


Copy link

Choose a reason for hiding this comment

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

To resolve - why could you not import bitcoin connect here? it's needed so zaps work on every page

Copy link

Choose a reason for hiding this comment

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

@Alan-AC7 For NextJS you can add this to src/components/PostItems/PostReactions/index.tsx to only import the library clientside:

useEffect(() => {
    // load bitcoin connect for zaps
    import("@getalby/bitcoin-connect-react");
  }, []);

@@ -162,3 +162,14 @@
text-overflow: ellipsis;
white-space: nowrap;
}

:root {
--bc-color-brand: rgb(89,128,34);
Copy link

Choose a reason for hiding this comment

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

why do you need to re-declare the color? isn't it already declared somewhere in the site's css? is it one of the primary hex colors?

@Alan-AC7 Alan-AC7 changed the title added btc component on profile preferences add Bitcoin Connect Dec 5, 2023
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.

5 participants