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

feat: Make lnlambda.lnmetrics.info default lamdaServer url #143

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mdmohsin7
Copy link
Contributor

While setting up connection type of ClientMode.lnlambda, the user has to provide a URL for a lambda server. Those who aren't aware of any, they can use lnlambda.lnmetrics.info, but this thing isn't documented anywhere. Hence this PR makes https://lnlambda.lnmetrics.info the default option for lambda server which the user can change if they want to.

CLN App Web Screenshot

Copy link

netlify bot commented Feb 18, 2024

Deploy Preview for clnapp failed.

Name Link
🔨 Latest commit f1c0f5b
🔍 Latest deploy log https://app.netlify.com/sites/clnapp/deploys/665a0873dd249c0008d2723c

@Harshit933 Harshit933 self-assigned this Feb 19, 2024
@Harshit933 Harshit933 self-requested a review February 19, 2024 10:02
Copy link
Collaborator

@Harshit933 Harshit933 left a comment

Choose a reason for hiding this comment

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

ACK 7147ea5

@Harshit933 Harshit933 removed their assignment Feb 19, 2024
Copy link
Member

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

This is not the solution that I had in mind, but it is just a default value.

The URL should be a choice of the user from a list of options

@@ -41,7 +41,7 @@ class _LnlambdaSettingViewState extends State<LnlambdaSettingView> {
),
const Text("Lambda Server"),
TextFormField(
controller: TextEditingController(text: setting.lambdaServer ?? ''),
controller: TextEditingController(text: setting.lambdaServer),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
controller: TextEditingController(text: setting.lambdaServer),
controller: TextEditingController(text: setting.lambdaServer),

we can use another component? maybe a PopupMenuButton https://api.flutter.dev/flutter/material/PopupMenuButton-class.html or something similar?

@mdmohsin7
Copy link
Contributor Author

This is not the solution that I had in mind, but it is just a default value.

The URL should be a choice of the user from a list of options

Yes that can be done. We can have a dropdown from which the user can select one of the many lambda URLs. Among the options in the dropdown, one option can be enter manually. If user selects the enter manually option, then we will show a text field for the user to enter the url. Else we will go with the user selected URL from the dropdown.

@vincenzopalazzo
Copy link
Member

Sounds good

@mdmohsin7
Copy link
Contributor Author

Yes that can be done. We can have a dropdown from which the user can select one of the many lambda URLs. Among the options in the dropdown, one option can be enter manually. If user selects the enter manually option, then we will show a text field for the user to enter the url. Else we will go with the user selected URL from the dropdown.

Here's a demo video of how this will work

CLN.App.-.20.February.2024.mp4

@vincenzopalazzo
Copy link
Member

@mdmohsin7 instead to set the first value to an empty one, can you set it to the lnmetrics URL?

Thanks

@mdmohsin7
Copy link
Contributor Author

@mdmohsin7 instead to set the first value to an empty one, can you set it to the lnmetrics URL?

Thanks

This is done.
Attaching a demo video of it.

CLN.App.-.13.March.2024.mp4

@vincenzopalazzo vincenzopalazzo force-pushed the main branch 2 times, most recently from 176d4dd to 93e980e Compare May 8, 2024 21:27
Copy link
Member

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Thanks this looks good.

Can you address the last review that I left?

then if you can rebase on top of master will be a good step, thanks

lib/components/bottomsheet.dart Outdated Show resolved Hide resolved
Co-authored-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
@vincenzopalazzo
Copy link
Member

looks like that it needs a rebase

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.

3 participants