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

Ability to add agent url on config screen #92

Merged
merged 15 commits into from
Feb 19, 2024

Conversation

HunnySajid
Copy link
Collaborator

#91

@HunnySajid
Copy link
Collaborator Author

@rodolfomiranda @2byrds
The agent URL is added to the config screen. The user can not proceed without adding the vendor and agent URLs.
The default Keria theme is also set at src/config/vendor.json

Screen.Recording.2024-02-13.at.11.23.13.PM.mov

@2byrds
Copy link
Collaborator

2byrds commented Feb 13, 2024

@HunnySajid if they provide the vendor URL it should pre-populate the keria URL in most cases? So I think the keria URL should only be available if they have no vendor URL or if the vendor URL JSON does not provide a keria agent. Thoughts @rodolfomiranda ?

@rodolfomiranda
Copy link
Collaborator

@HunnySajid if they provide the vendor URL it should pre-populate the keria URL in most cases? So I think the keria URL should only be available if they have no vendor URL or if the vendor URL JSON does not provide a keria agent. Thoughts @rodolfomiranda ?

agree. The normal case is to enter the vendor URL, but we should provide an option for users that use own KERIA instances. A small link with that option may work

@2byrds
Copy link
Collaborator

2byrds commented Feb 13, 2024

@HunnySajid if they provide the vendor URL it should pre-populate the keria URL in most cases? So I think the keria URL should only be available if they have no vendor URL or if the vendor URL JSON does not provide a keria agent. Thoughts @rodolfomiranda ?

agree. The normal case is to enter the vendor URL, but we should provide an option for users that use own KERIA instances. A small link with that option may work

That sounds good. I think if the vendor URL gives you a keria agent then it should be filled in. And you see it IF YOU click the link. If the vendor URL doesn't supply it, then the link automatically opens and the user has to fill it in.

@HunnySajid
Copy link
Collaborator Author

@HunnySajid if they provide the vendor URL it should pre-populate the keria URL in most cases? So I think the keria URL should only be available if they have no vendor URL or if the vendor URL JSON does not provide a keria agent. Thoughts @rodolfomiranda ?

agree. The normal case is to enter the vendor URL, but we should provide an option for users that use own KERIA instances. A small link with that option may work

@rodolfomiranda
Even in that case, user needs to load vendor JSON otherwise they can not proceed.
Let's put an array of string agentUrls.

@HunnySajid
Copy link
Collaborator Author

@rodolfomiranda @2byrds

In the previous flow, it was cumbersome for the user to type/paste multiple URLs. e.g vendor_url then agent_url.
Also, it was not seamless as the popup was getting close between copy pasting.

I have updated the flow, now agentUrls are a part of vendor JSON as an array of string.

"agentUrls": ["https://keria-dev.rootsid.cloud/admin", "https://keria-dev.rootsid.cloud/fakeone"]

If vendor json does not have agentUrls, the extension throws an error

JSON loaded without agentUrls

Screen.Recording.2024-02-14.at.11.33.53.PM.mov

If vendor JSON has agentUrls then the first URL is selected and flow continues, user can change the URL from dropdown later on:

JSON loaded with agentUrls

Screen.Recording.2024-02-14.at.11.34.27.PM.mov

Let me know how it looks.

@2byrds
Copy link
Collaborator

2byrds commented Feb 14, 2024

@HunnySajid thank you for posting a look. Does the vendor URL have to have an agent URL? I don't think that should be mandatory, so no error if it's missing. Instead if there is no agent URL the UI makes the user fill it in. Also the user should have the option to fill it in even if the vendor does provide it, as u showed.

@rodolfomiranda
Copy link
Collaborator

What's the case for a vendor URL to not have a agent URL? also, what's the purpose of having multiple agent URLs in the json?

@HunnySajid
Copy link
Collaborator Author

What's the case for a vendor URL to not have a agent URL? also, what's the purpose of having multiple agent URLs in the json?

What's the case for a vendor URL to not have a agent URL?
The assumption is vendor would provide an agentUrl or a list of agentUrls to developers. I saw Lance's comment that it is not mandatory for vendors to provide agentUrl to developers. The devs can connect with any agent so they can fill in the agentUrl field as well.

what's the purpose of having multiple agent URLs in the json
The assumption was that vendors would provide a list of agentUrls and developer can connect with any of the listed agents.

Now, I am making following changes:

  • remove agentsUrl from JSON and use one optional field named agentUrl.
  • instead of dropdown for agentUrl selection, use a text field which could be edited by user.

@rodolfomiranda
Copy link
Collaborator

I imagine that the vendor URL will be use for real users that were onboarded previously by the vendior. Only devs or expert users may need to use a custom agentURL, and vendor theming shouldn't be needed.

@HunnySajid
Copy link
Collaborator Author

@rodolfomiranda i have made updates based on the conversation. Please review

@HunnySajid
Copy link
Collaborator Author

@HunnySajid
Copy link
Collaborator Author

Demo review changes: #93

@HunnySajid HunnySajid mentioned this pull request Feb 15, 2024
8 tasks
Copy link
Collaborator

@2byrds 2byrds left a comment

Choose a reason for hiding this comment

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

LGTM

@rodolfomiranda rodolfomiranda merged commit 7db50ce into WebOfTrust:main Feb 19, 2024
@HunnySajid HunnySajid deleted the feat/agent-url branch February 19, 2024 23:04
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