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 logic to connect to Portal workspace automatically #28

Merged
merged 19 commits into from
Oct 17, 2022
Merged

Conversation

mivasconcelos
Copy link
Contributor

@mivasconcelos mivasconcelos commented Oct 6, 2022

This logic aims to integrate this app with Portal and connect them automatically.

  • [Portal] user clicks on "go to application"

  • [Portal] it's created a custom user to be used on the RTDM app

  • [RTDM] extracts hostname and credentials from the URL

  • [RTDM] establishes a connection automatically with those values

https://www.loom.com/share/428a049fb7b64302affdf1bdc10e4181
cc: @davidgomes

web/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@carlsverre carlsverre left a comment

Choose a reason for hiding this comment

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

Comments

web/package.json Outdated Show resolved Hide resolved
web/src/use-portal-connection.tsx Outdated Show resolved Hide resolved
web/src/components/WelcomeModal.tsx Outdated Show resolved Hide resolved
@carlsverre
Copy link
Contributor

Note, any changes to RTDM must be backwards compatible with vapor and manual configuration. Hence why I need you to follow how the vapor config works.

You will also need to check to see if there is a portalConfig in useConnectionState (hooks.tsx). Probably best to refactor any of the code that's keyed off isVapor to be something like isManaged to show different configuration panels to the user in various places (overview page, database drawer).

Copy link
Contributor

@carlsverre carlsverre left a comment

Choose a reason for hiding this comment

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

removed comment

@carlsverre
Copy link
Contributor

It doesn't appear that my previous comments have been addressed in the current state of this diff.

@carlsverre
Copy link
Contributor

You are missing updating the part of the app that checks for vaporConnectionConfig to change how the UI looks (to show that the connection is already configured). See my comment above:

You will also need to check to see if there is a portalConfig in useConnectionState (hooks.tsx). Probably best to refactor any of the code that's keyed off isVapor to be something like isManaged to show different configuration panels to the user in various places (overview page, database drawer).

web/src/data/recoil.ts Outdated Show resolved Hide resolved
web/src/data/recoil.ts Show resolved Hide resolved
@mivasconcelos
Copy link
Contributor Author

mivasconcelos commented Oct 14, 2022

You are missing updating the part of the app that checks for vaporConnectionConfig to change how the UI looks (to show that the connection is already configured). See my comment above:

I don't think we want to change how the UI looks - at least for now. We should keep it how it is. I believe it's ok to show the connection form. The design team is working on a new UI for this app though.

Copy link
Contributor

@carlsverre carlsverre left a comment

Choose a reason for hiding this comment

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

Can you create an updated loom of this flow

web/src/data/recoil.ts Outdated Show resolved Hide resolved
web/src/data/recoil.ts Outdated Show resolved Hide resolved
web/src/data/recoil.ts Show resolved Hide resolved
Copy link
Contributor

@carlsverre carlsverre left a comment

Choose a reason for hiding this comment

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

I think this looks solid. Nice job

@carlsverre carlsverre merged commit 787bef1 into main Oct 17, 2022
@carlsverre carlsverre deleted the mcdb-12135 branch October 17, 2022 23:21
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.

2 participants