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

Custom user permissions UI #451

Merged
merged 9 commits into from
Jan 20, 2023
Merged

Custom user permissions UI #451

merged 9 commits into from
Jan 20, 2023

Conversation

itsrachelfish
Copy link
Contributor

@itsrachelfish itsrachelfish commented Nov 4, 2022

This PR updates the Lightning Node Connect page to include a new overlay which allows users to create macaroons with customized permissions for specific URIs as well as expiration dates.

New option added to permissions dropdown

image

Custom permissions overlay

image

Custom session with an expiration date

image

Related to: #438
Depends on #450 & #457

@dstrukt dstrukt self-requested a review November 4, 2022 23:04
@jbrill
Copy link

jbrill commented Nov 22, 2022

One future thing that could be helpful is to create a common component library as a node package for us to consume in both terminal and lit. This way we could avoid duplicated code, and maybe create a storybook for @dstrukt to view what's being used in the codebase

@jbrill
Copy link

jbrill commented Nov 22, 2022

Should Liquidity Manager and Payments Manager permission roles be in the dropdown as well?

@levmi
Copy link
Contributor

levmi commented Nov 22, 2022

Should Liquidity Manager and Payments Manager permission roles be in the dropdown as well?

Design/product decision to just have those live in custom as well. Want to see if people use them or not. Will populate the dropdown with the most popular options based on feedback. Still unsure how popular those two will be, but it's our first stab at it.

@jbrill
Copy link

jbrill commented Nov 22, 2022

I can imagine the terminal lift will be significant for some of these custom permissions -- would those changes happen before the merge? Just curious

@jbrill
Copy link

jbrill commented Nov 22, 2022

The code looks very clean and can confirm the feature works very well 👍

Will do a more thorough pass when it's ready for review!

@dstrukt
Copy link
Contributor

dstrukt commented Nov 22, 2022

Initial run through looks great @itsrachelfish awesome work .. could definitely be code-reviewed at this stage!

Will type up more notes and nits.

@dstrukt
Copy link
Contributor

dstrukt commented Nov 23, 2022

One future thing that could be helpful is to create a common component library as a node package for us to consume in both terminal and lit. This way we could avoid duplicated code, and maybe create a storybook for @dstrukt to view what's being used in the codebase

This is interesting .. ACK for the shared library, and storybook if we keep it low effort for time being.

@itsrachelfish itsrachelfish force-pushed the custom-connect-ui branch 2 times, most recently from 37c5229 to 8629c38 Compare November 28, 2022 09:46
@itsrachelfish itsrachelfish changed the title WIP - Custom user permissions UI Custom user permissions UI Nov 28, 2022
@itsrachelfish itsrachelfish marked this pull request as ready for review November 28, 2022 09:56
app/src/store/views/addSessionView.ts Outdated Show resolved Hide resolved
@dstrukt
Copy link
Contributor

dstrukt commented Nov 28, 2022

Design Review:

  • Add 24px padding above Proxy Server input

Screen Shot 2022-11-28 at 12 10 16 PM

  • Make the calendar icon our offWhite

Screen Shot 2022-11-28 at 12 14 24 PM

  • Make the calendar text copy click trigger the dropdown, only the icon does this (ideal, but not required)

  • Conversely, the icon click on the select dropdown should trigger the dropdown too .. but this is an issue across LITD/TW, so another ideal, but not required, and might be worth a sep. issue, but defer to you

  • Add a 200ms ease-in-out transition to the background-color when hovering/selecting a Permission Type (just a nice to have)

Copy link
Contributor

@dstrukt dstrukt left a comment

Choose a reason for hiding this comment

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

See above.

@itsrachelfish
Copy link
Contributor Author

@dstrukt The calendar icon for the date picker is added automatically by chrome, and the click behavior is controlled by the browser as well. When you test in firefox there is no date picker icon and the entire input field is clickable. I was able to change the color of the date picker icon but not much else

@itsrachelfish itsrachelfish force-pushed the custom-connect-ui branch 2 times, most recently from 1b5bb29 to 54dc1f3 Compare December 1, 2022 22:24
Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

tACK 🔥 Tested many different permission configurations and didn't run into any issues.

Overall, the code looks great. I commented with a bunch of nits which aren't blocking this from being merged. Just some minor improvements to make it easy to grasp what's happening in the code.

app/src/store/views/addSessionView.ts Outdated Show resolved Hide resolved
app/src/store/views/addSessionView.ts Outdated Show resolved Hide resolved
app/src/store/views/addSessionView.ts Outdated Show resolved Hide resolved
app/src/store/views/addSessionView.ts Outdated Show resolved Hide resolved
app/src/store/views/addSessionView.ts Outdated Show resolved Hide resolved
app/src/store/views/addSessionView.ts Show resolved Hide resolved
app/src/components/connect/CustomSessionPage.tsx Outdated Show resolved Hide resolved
app/src/components/connect/CustomSessionPage.tsx Outdated Show resolved Hide resolved
@dstrukt dstrukt self-requested a review December 2, 2022 19:18
Copy link
Contributor

@dstrukt dstrukt left a comment

Choose a reason for hiding this comment

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

Changes LGTM!

Excellent work!

@dstrukt
Copy link
Contributor

dstrukt commented Dec 6, 2022

Only other thing I noticed .. shouldn't we generate a generic label "My Custom Session" or whatever?

Or am I missing something?

cc @levmi @itsrachelfish

Screen Shot 2022-12-05 at 4 34 02 PM

Screen Shot 2022-12-05 at 4 34 37 PM

@itsrachelfish
Copy link
Contributor Author

@dstrukt I added some code to automatically generate labels when none are defined. Users can still specify their own label when first viewing the add session option. However if no label is specified, the custom permissions page will automatically generate a label based on the permission type selected.

"My read-only session", "My liquidity session", "My loop session", etc.

@itsrachelfish itsrachelfish force-pushed the custom-connect-ui branch 2 times, most recently from f40bcc4 to c87fd62 Compare December 12, 2022 09:22
@jamaljsr
Copy link
Member

Thanks for the updates. The latest commit LGTM 🚀

Copy link
Contributor

@levmi levmi left a comment

Choose a reason for hiding this comment

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

All LGTM, great work!

@ellemouton
Copy link
Member

needs a rebase :)

@itsrachelfish itsrachelfish force-pushed the custom-connect-ui branch 2 times, most recently from 863f5b0 to 4665cf3 Compare January 12, 2023 00:36
@itsrachelfish
Copy link
Contributor Author

@levmi @ellemouton updated with the latest from master 👍

Copy link
Member

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

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

tACK 🔥

@guggero
Copy link
Member

guggero commented Jan 13, 2023

@itsrachelfish do you want to rebase to get a clean commit history (all authored and committed by you) or should we just merge?

@lightninglabs-deploy
Copy link

@itsrachelfish, remember to re-request review from reviewers when ready

@guggero guggero merged commit 3b58e8e into master Jan 20, 2023
@guggero guggero deleted the custom-connect-ui branch January 20, 2023 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants