Skip to content

Add configuration panel for Application Credentials#12344

Merged
bramkragten merged 14 commits intohome-assistant:devfrom
allenporter:app_credentials
May 9, 2022
Merged

Add configuration panel for Application Credentials#12344
bramkragten merged 14 commits intohome-assistant:devfrom
allenporter:app_credentials

Conversation

@allenporter
Copy link
Contributor

@allenporter allenporter commented Apr 17, 2022

Proposed change

Add a configuration panel for Application Credentials which allow users to configure OAuth Client ID and OAuth Client
Secret for an integration using the user interface.

Work in progress:

  • determine correct place for this in the UI (arbitrary placeholder for now, asked for input in #devs_us)
  • add core support for an integration (e.g. xbox)
  • add core websocket for returning supported integrations
  • render integrations drop down or search in fronted UI
  • support for deleting an integration
  • special display of imported credentials vs user added credentials

Capture

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • 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

Checklist

  • The code change is tested and works locally.
  • 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:

@zsarnett
Copy link
Contributor

@matthiasdebaat Let's talk about this. Not sure where this should be. The configuration sections are all changing so this will definitely not stay where is but maybe move to the system tab 🤷

@allenporter
Copy link
Contributor Author

@matthiasdebaat Let's talk about this. Not sure where this should be. The configuration sections are all changing so this will definitely not stay where is but maybe move to the system tab 🤷

Thanks, really appreciate this. I'll continue to work on the functionality, thought just wanted to flag this early.

@allenporter
Copy link
Contributor Author

@matthiasdebaat Let's talk about this. Not sure where this should be. The configuration sections are all changing so this will definitely not stay where is but maybe move to the system tab 🤷

Thanks, really appreciate this. I'll continue to work on the functionality, thought just wanted to flag this early.

@zsarnett i saw you landed a config revamp. Do you have a more well formed opinion now of where this should land, or otherwise have any principles regarding the most recent revamp?

I still have other pieces to iterate on fix up but thought I would ask while this was top of mind.

@allenporter
Copy link
Contributor Author

@matthiasdebaat Let's talk about this. Not sure where this should be. The configuration sections are all changing so this will definitely not stay where is but maybe move to the system tab 🤷

Thanks, really appreciate this. I'll continue to work on the functionality, thought just wanted to flag this early.

@zsarnett i saw you landed a config revamp. Do you have a more well formed opinion now of where this should land, or otherwise have any principles regarding the most recent revamp?

I still have other pieces to iterate on fix up but thought I would ask while this was top of mind.

I just rebased and can see how System makes sense given it seems like we've found a place to put low level things like this. I'm going to go ahead and move this there optimistically, given I think I can see where you're going with this, but happy to put this wherever makes sense.

@zsarnett
Copy link
Contributor

Hey @allenporter , Sorry I should have sent a message yesterday. We believe the best place for this will be in an Overflow menu on the Integrations, Devices, Entities, and Helpers page. The overflow menu can link your configuration page but will not be included in the tabs above

@allenporter
Copy link
Contributor Author

Oh no problem! Thanks for that, it makes sense to me as this is definitely integration related, and the only thing holding be back from placing it there was that it didn't seem anywhere near big enough to be on its own tab.

My frontend development skills are definitely still in the range of "copying from existing examples". Is there prior art you can point me at I can borrow from? or maybe i can grab time with you to discuss in more detail an approach? As an example of the types if things i'm not sure how to navigate (so to speak): i was playing with this in server section and had a little trouble figuring out the right layout to use with the right set of things i need (e.g. fab, tabs, data table, header labels, etc) as nothing existing seemed to have that combination, then had a little trouble knowing when to reuse vs when to go custom.

@zsarnett
Copy link
Contributor

@zsarnett
Copy link
Contributor

zsarnett commented Apr 21, 2022

Something like this

  <ha-button-menu
    corner="BOTTOM_START"
    slot="toolbar-icon"
    activatable
  >
    <ha-icon-button
      slot="trigger"
      .label=${this.hass.localize("ui.common.menu")}
      .path=${mdiDotsVertical}
    ></ha-icon-button>
    <ha-clickable-list-item
      @click=${this._entryClicked}
      href="/config/lovelace/resources"
      aria-label=${this.hass.localize(
        "ui.panel.config.lovelace.resources.caption"
      )}
    >
      ${this.hass.localize(
        "ui.panel.config.lovelace.resources.caption"
      )}
    </ha-clickable-list-item>
  </ha-button-menu>

@allenporter
Copy link
Contributor Author

allenporter commented Apr 23, 2022

Thank you that was extremely helpful. I am hitting a couple layout issues, still working through.

The overflow menu looks good ✅
Overflow menu

though it does not expand quite right ❓
Overflow menu expanded issue

And in narrow mode it looks good on 3 tabs ✅
Narrow overflow

However on the integrations tab it gets out of place ❌
Narrow overflow integrations issue

@zsarnett
Copy link
Contributor

I'll try to take a look this weekend or Monday

@zsarnett zsarnett self-assigned this Apr 23, 2022
@allenporter
Copy link
Contributor Author

No rush, I also need to do one last selector for an integration drop down

@matthiasdebaat
Copy link
Member

Awesome work @allenporter!

@zsarnett
Copy link
Contributor

I messed up the git commands but should be all fixed up now. Your commits are all the first commit now... Sorry about that. But the overflow should be good

@allenporter
Copy link
Contributor Author

@zsarnett 🔥 🔥🔥🔥!!!! thank you, so kind of you. I now will work on making the integration selector and take out of draft.

@allenporter allenporter marked this pull request as ready for review April 26, 2022 14:56
@zsarnett zsarnett marked this pull request as draft April 26, 2022 23:30
@zsarnett
Copy link
Contributor

When the backend gets merged can you set it back to ready for review?

@allenporter allenporter marked this pull request as ready for review April 28, 2022 05:01
@allenporter
Copy link
Contributor Author

When the backend gets merged can you set it back to ready for review?

Core PR said: Great! Can be merged when frontend PR is happy with this.
home-assistant/core#69148 (review)

So figured i'd get your pulse before actually merging.

@zsarnett zsarnett self-requested a review April 28, 2022 13:42
bramkragten
bramkragten previously approved these changes Apr 29, 2022
yarn prettier --write src/panels/config/application_credentials/dialog-add-application-credential.ts
@allenporter
Copy link
Contributor Author

Whoops, noticed I had a lint error, fixed.

@bramkragten bramkragten merged commit 00c5d3d into home-assistant:dev May 9, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 10, 2022
@allenporter allenporter deleted the app_credentials branch July 9, 2022 15:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants