-
Notifications
You must be signed in to change notification settings - Fork 285
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(ledger-browser): implement dynamic app setup #3401
Conversation
This PR/issue depends on:
|
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
- Read gui supabase connection information from environment variable. Include `.env` files in common `.gitignore` file. Change ledger-browser typescript target and module to `esnext` to use vite environment variables. - Read app configuration from the supabase DB. - Add button on home page for adding new application. Clicking on it will open dialog with setup wizzard. User must filter apps by it's group (step 1), select the app (step 2), input common app configuration data (step 3) and lastly input app-specific configuration (JSON format). - Add button to configure already added app. It opens a dialog that allows editing app details in the database. It also contains a button for deleting the app (after confirmation). - Show full screen error message with setup guidelines when app has failed to connect to supabase. - Clean up supabase type files, move app-related typedefs to specific app dirs. Depends on hyperledger-cacti#3347 Signed-off-by: Michal Bajer <michal.bajer@fujitsu.com>
eaea4aa
to
00c6c35
Compare
@petermetz @jagpreetsinghsasan I can't make |
@outSH sorry for the blocker. I just now checked and it seems that there was one more regex needed before matching the pr body to the commit array elements (An extra quote was there in the striped strings). I will create a PR to address this. Can we not merge this PR if its urgent as I will test some more scenarios before actually pushing the fix? |
@jagpreetsinghsasan I'm good with merging this as-is, let's not treat it as a blocker until we have high confidence that the check is working well and is not just being annoying people with being pedantic and overly strict. So I'd say, in general, that the the PR commit parity check is still in beta testing and so I haven't made it mandatory for now in the branch protection rules either. And I have been merging pull requests that fail the check too because I couldn't figure it out, see => https://github.com/hyperledger/cacti/issues/3470 for details on my adventures. |
@outSH Thank you for looking out for the check and trying to make it work and sorry that it's not perfect yet! I put a bunch more thoughts in my comment above, but long story short: I'm good to merge as-is and sorry about the confusion! |
feat(ledger-browser): implement dynamic app setup
Include
.env
files in common.gitignore
file.Change ledger-browser typescript target and module to
esnext
to usevite environment variables.
Clicking on it will open dialog with setup wizzard.
User must filter apps by it's group (step 1), select the app (step 2),
input common app configuration data (step 3)
and lastly input app-specific configuration (JSON format).
editing app details in the database.
It also contains a button for deleting the app (after confirmation).
when app has failed to connect to supabase.
Depends on #3347
Pull Request Requirements
upstream/main
branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.