-
Notifications
You must be signed in to change notification settings - Fork 109
ui: add Terminal and Amboss to Lightning explorer options #467
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
Conversation
guggero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, definitely nice to have amboss.space as an option, don't have a strong opinion on making it the default though.
jamaljsr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK LGTM 👍
Agreed that amboss should certainly be an option.
We are discussing internally what to set the default to as we'd like to also have https://terminal.lightning.engineering/ as an option as well. I'm going to hold off on merging this until that decision is made by the team.
|
Understood! Thanks for checking the PR. As long as we move away from 1ml 😁 |
|
@apotdevin, remember to re-request review from reviewers when ready |
|
Hey @apotdevin we've decided we should add Amboss and Terminal, but make Terminal the default. If you would like to make these changes in the PR, please feel free to. Otherwise, you can close this and I will follow-up with a new PR with the changes. Thanks for sparking the convo to help us get this updated. Appreciate it 👍 |
|
Sounds good! Closing this one then |
|
Reopening this one with the suggested change! |
jamaljsr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK LGTM 👍
Apologies for forgetting about this one. Thanks for resurfacing it.
I tested and confirmed it's working as expected. Just needs a small fix then can be merged.
|
No worries! Should have just done the change back then |
guggero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, LGTM 🎉
jamaljsr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
Thanks again for the update. I just squashed the commits before I merge this.
Change to give users a better experience when getting information for other nodes.