SSH: add reattach key action #553
Conversation
| {% block top_menu %} | ||
| <div class="ui stackable text menu"> |
There was a problem hiding this comment.
This section is always for adding objects to a listing of objects, with a few exceptions for export/download buttons. I would not repurpose this to fit this UI or other one-off buttons and would move this UI outside this partial template and put the content in a .ui.segment.
This also fits in the listing as a small button menu for each key, other UIs use this approach and it would fit if we make SSH keys many to many
There was a problem hiding this comment.
It would be easier to do this the first time if we had documented patterns or guidelines for this stuff. Whenever I edit the templates it feels like I'm just guessing at where to put stuff and what classes to use, and then just waiting to be told I'm doing it wrong. There has to be a better pattern.
There was a problem hiding this comment.
I wrote a lot of documentation already, this template and pattern especially:
https://docs.ops.verbthenouns.com/projects/ext-theme/en/latest/api/templates.html#shared-list-base
| {% blocktrans trimmed %} | ||
| Alternatively, you can | ||
| <a href="https://docs.readthedocs.io/en/stable/guides/importing-private-repositories.html#copy-your-project-s-public-key">manually add the SSH key to your Git provider</a>. | ||
| {% endblocktrans %} |
There was a problem hiding this comment.
I would not give two separate actions in this UI, I'd drop this suggestion. It can be a help topic if we need but the first UI element shouldn't point users to manually manage this.
| </div> | ||
|
|
||
| {% if not can_reattach_key_automatically %} | ||
| <div class="ui warning message"> |
There was a problem hiding this comment.
This feels like an error state the user needs to fix. We don't want to send the user down the rabbit hole of fixing the connection to a repository unless their builds aren't working though.
The user isn't going to know of what the difference is between reattaching and "can't automatically reattach", this is fairly technical.
I think this content should be reframed a bit. If the user can reattach an SSH key, the message around this button should be "If you are having trouble building, you can reattach the SSH key". This is missing from the explanation right now.
In the case the user can't reattach, I don't think it should be an error about not being able to reattach, but instead "To enable SSH key management, connect a repository". That is, avoid describing this visually and with copy as an error state.
All of this might fit better in a modal really, but either options works for a first pass.
|
@agjohnson so from what I understand:
|
Yeah, using a listing was purposely looking forward, though the legacy UI did this too. It keeps the admin UIs consistent, where almost every sub page is a listing. Also, we have talked about allowing users to manage their own SSH keys, which would drive this to look like a listing anyways.
I think cleaning up what you have works too, but the above matches much closer what other UIs already do and this wouldn't need a second pass later on.
Probably not necessary, that view is just the public key. I suspect the detail view would be better as a "Copy SSH key" button even, but that's separate.
Perhaps even just always show a modal -- confirm the change if possible or warn reconnection is not possible if not? |
|
I found this PR while preparing the release. What is missing to finish it? |
ref https://github.com/readthedocs/readthedocs-corporate/pull/1926
Also, it's a little weird that we are using a listing UI, when we have support for only one key at the moment.