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

Try/settings page manager #46

Merged
merged 26 commits into from
Jun 6, 2019
Merged

Try/settings page manager #46

merged 26 commits into from
Jun 6, 2019

Conversation

ryanwelcher
Copy link
Contributor

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Verification Process

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

@jeffpaul jeffpaul added this to the 1.3.0 milestone Apr 1, 2019
@ryanwelcher
Copy link
Contributor Author

ryanwelcher commented May 17, 2019

@helen my approach to the settings screens are to have a page for the overall settings for ClassifAI and one for each of the Services the plugin provides:

classifai-services

Each of the Services pages has a tabbed interface for the settings of each Provider:

classifai-image-processing-tab

Having a tab for each Provider take care of the issue of having all of the settings on the same page especially if there is a large number of settings:

classifai-nlu-page

This branch has the new settings system in place and has Watson refactored to work as expected. Each provider stores its settings as a discrete option. This will allow for better clean up by not having to do any surgery on a serialized array of settings.

The only part I have not tackled is how to handle enabling/disabling services. This can be managed indirectly by not having credentials stored for the Provider but I am wondering if having a dedicated on/off is a better idea.

@jeffpaul
Copy link
Member

Does anyone foresee a scenario where someone would want more than one Service Provider for a particular Service (e.g., Watson & Azure for Image Processing)? I suppose its possible, but seems like a low probability and thus feel we can make things easier on ourselves if we expect only one Service Provider to be enabled per Service. Thus having some sort of selection of the Service Provider that then presents the settings for that provider versus showing all available Service Provider settings in tabs make be less confusing? @helen what's your UX take on this?

@jeffpaul jeffpaul requested a review from helen May 17, 2019 19:50
@jeffpaul jeffpaul added the type:enhancement New feature or request. label May 17, 2019
@ryanwelcher
Copy link
Contributor Author

ryanwelcher commented May 20, 2019

@jeffpaul @helen I was thinking about this over the weekend and I think that for each feature we introduce, there really can only be one service.

For example, if we're using Image Processing to generate alt tags programmatically, we could pass the image to all of the configured Providers but there would be no way, beyond manually inspecting the results, to determine which is the best. With a single Provider, the one with the highest confidence would be the best.

Additionally, it might be a good idea to introduce a Features page where we can pick the Provider that we're using for each feature. There is definitely the possibility that a user might what to mix and match Image Processing Providers to generate alt tags, generate captions and do object recognition.

@helen
Copy link
Contributor

helen commented May 22, 2019

My thought at the moment (still subject to change, etc.) is that I like this direction and I don't think the core plugin needs to support multiple providers for a given service type. This isn't necessarily technical direction, just documenting thoughts here.

For a given service type, I think that workflow would be selecting a given provider (a select would be the MVP, something more visual like a row of logos or something might be better), inputting your credentials, and then moving into the rest of the settings if your credentials check out. A potential/likely issue here is whether various service types actually map cleanly between different providers - we should probably collate a list of service provider, service (language, image, etc.), functionality, and credential requirements to compare them. I can start on that. Otherwise we don't know if saying "enter your image service credentials for X" is actually accurate.

Related to that is when a given service does not have functionality that another does for that particular service. Available functionality would be shown because you should be able to toggle each bit on and off, but what about things that aren't available so you know what you're missing?

@jeffpaul
Copy link
Member

All good points @helen, a doc/spreadsheet showing the service providers coverage across the service types and features therein is definitely worth starting (I can also help there), and some basic UX noting how we'd display that mapping within the settings is also probably something to get someone with availability to assist on once we have that mapping listed out. For now and the near future, we're going to have a 1-to-1 mapping of service provider and service type, so standing up something simple for now is best. We can work through all the wacky combinations and a graceful UX to support that once we determine the service providers and/or services we add next.

@jeffpaul
Copy link
Member

I think these settings changes are a good start and can be used in v1.3.0. Now just needs to add the Azure settings for the Image Processing integration, presumably via a separate PR from @helen.

@helen helen merged commit a888aeb into develop Jun 6, 2019
@jeffpaul jeffpaul deleted the try/settings-page-manager branch March 5, 2020 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants