Add migration page for GitHub App #570
Conversation
readthedocsext/theme/templates/profiles/private/migrate_to_gh_app.html
Outdated
Show resolved
Hide resolved
|
We need to upgrade Fomantic to the latest version in order to test the circled layout, but wasn't able to find formantic in the package.json file.... |
|
Thinking of manually implementing the checkmark (not sure how), as the builtin doesn't play nice when the description is longer than a few words
Or maybe just don't use icons at all... Maybe the circular form doesn't have this problem |
|
This looks great, awesome job! I just had some minor notes on the visual example so far, I'll take a look at the code next. This listing is going to be really cramped with the warning and button. It will easily wrap (in unexpected ways) if the repo or project name is longer
I think we should either show just the warning or just the "Install" button. What you are trying to communicate is that the user skipped a step and didn't install the GHA, but "Project can't be migrated" doesn't explain this either. Having an "Install" button to effectively go back a step is okay too -- it does seem a little confusing why there are two steps to install our application though. With the steps split up by this, replacing the warning and the "Install" button with a warning "GitHub App not installed". The user would understand why they can't migrate the project with that warning, and they should go back to install the application. Also, we have an element for the repo tag style already:
This should be a reusable template:
Instead of using disabled buttons to indicate successful states, I would use a label with a green check:
Or, just "Installed" is fine too. The disabled buttons aren't meant to be readable and they do not pass accessibility contrast standards. Lastly, toast notifications communicating errors within views are too easy to miss. We aren't exposing errors via toast notifications anywhere right now and I think that is best. There are better options to push the user away from entering the wizard twice, like showing an error state on the button that the user would click to enter this wizard. However, is there a reason we don't want the user able to come back to this UI? I'm sure there will be legitimate use cases where the user can't complete this wizard for all repos -- for example a user can't reach the organization admins in time. I think the best UX for this would be to show the first steps as completed if the user comes back to this UI. |
This is because we share some styles and code between our website and application, we have a common repo for this: Line 29 in 4251fe1 |
There are two possible reasons a project can't be migrated, one is the app not being installed into the app, and the other one is the user not having admin access to the repo. So we need a way to communicate both errors.
The previous install step allows you to install the app in all the repos at once, the install button on the migrate page allows you to install one repo at a time. I thought about other options, but wasn't convinced they were good options:
The toast notification is meant for users that go directly to that URL.
Users don't need to migrate all repos at once, they can go back to the wizard any time. The wizard should be accessible until the user completes the last two steps (revoke and disconnect). Once the user has revoked/disconnected access to the old GH OAuth app, there is nothing left to do from the wizard, as we can't migrate the projects anymore. |
|
Something like this? Not sure the way the repo is displayed integrates well with the listing
|
I would use
This is another error state that should be communicated clearer. Do we even want an install button if there is a problem with permissions? Removing the button and listing the specific reasons why the project can't be migrated would be most useful here. We should avoid cramming multiple elements like this into the listing view as it will break in a number of different cases, especially with the side bar menu.
Yeah I picked up on that, but we still should not use toast notifications for this. Users should get a proper view UI where we explain the error state. The users shouldn't be dropped on the dashboard page, as this is unexpected and confusing.
What I meant was I would expect that if I went through the wizard I'd still at least be able to see which projects weren't migrated yet. I tried to test this locally but it has required a lot of set up so far and I don't quite have it configured -- I think the last piece I need is a development GHA. |
Is that in all places? like even in listings?
The repo must be installed first in order for us to know if the user has admin permissions on it.
Not sure where would that list fit in the steps, like the steps are designed to follow each one in order, after the old social account has been revoked, it doesn't make sense displaying all the steps, and the migrate step where the repos missing migration are listed wouldn't make sense either as the user can't use the migrate option anymore. |
|
I was able to decouple the old GitHub account from the migration process. So users can still user the migration page after disconnecting the account, or rather, we can still use that same code, since I'm still not sure how to display the steps after the old account has been disconnected. Maybe don't display the revoke/disconnect steps, and change the wording to mention that manual steps may be required for the user to remove the webhook/ssh key from the repo. |
|
Also, what about using an icon to indicate that a project can't be migrated? If the user hovers over that icon we show what's required
Another idea would be to have four listings:
|
While this still requires readthedocs/readthedocs.org#11942, I think it should be safe to review, as the UI shouldn't change much.
I still need to finishe a couple of things: make all strings translable, and check for users with multiple GH accounts connected.Screencast.From.2025-03-18.18-48-25.mp4