frontend icon indicating copy to clipboard operation
frontend copied to clipboard

Avoid starting config flow and show alert dialog early if single config entry only

Open jpbede opened this issue 1 year ago • 6 comments

Proposed change

Only show the "Add entry" button if the integration signals that it supports multiple entries, otherwise show the user a info that the integration only allows one entry; instead of starting a flow, and show the user that there is already an entry and that they can only configure one.

image

Type of change

  • [ ] Dependency upgrade
  • [ ] Bugfix (non-breaking change which fixes an issue)
  • [x] New feature (thank you!)
  • [ ] Breaking change (fix/feature causing existing functionality to break)
  • [ ] Code quality improvements to existing code or addition of tests

Example configuration


Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:
  • Link to core pull request: https://github.com/home-assistant/core/pull/109505

Checklist

  • [x] The code change is tested and works locally.
  • [x] There is no commented out code in this PR.
  • [ ] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

jpbede avatar Feb 03 '24 14:02 jpbede

From a UX perspective, I think this isn't correct?

I believe that the buttons/UX elements should always be there, but be either disabled or provide an error message with an explanation on click (or maybe even a combination of those?)

Anyways, looking at the screenshot, the original UX element is removed and replaced by an in-line warning/error/informational message, which breaks that consistency and adds clutter to the UI imho.

frenck avatar Feb 12 '24 19:02 frenck

Agreed, I re-added the button as disabled with the info below. Or should we handled this just with a dialog?

image

jpbede avatar Feb 12 '24 19:02 jpbede

Want there some cases where re setting up an integration would update the config on the old entry? It was the only way to retain history for entities.

elupus avatar Feb 15 '24 05:02 elupus

This is still possible, just don't set the manifest option. The option is meant for integrations like sun, which immediately abort if they already have a config entry.

jpbede avatar Feb 16 '24 07:02 jpbede

I believe that the buttons/UX elements should always be there, but be either disabled or provide an error message with an explanation on click (or maybe even a combination of those?)

Correct. UX guidelines say we should always show the button and not disabled. Clicking on it will show the error. This means that it would be the same behavior as it was before the manifest option.

I also think that showing a permanent warning is overkill. Most of the times people come to this page to do something else, and now their attention is always drawn to this option.

I would suggest that we limit the impact of this PR to update the types.

I think that the feature itself can still be beneficial, like limiting discovery.

balloob avatar Feb 27 '24 04:02 balloob

I would suggest that we limit the impact of this PR to update the types.

I've now restored the old behavior. But we need to at least add the alert dialogs, otherwise the user won't get any information if we use the new manifest option. Core no longer starts a flow if an entry already exists and the option is used.

jpbede avatar Feb 27 '24 05:02 jpbede