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.

Sign up for GitHub

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

[dashboard] show repo name when workspace creation fails #7302

Merged
merged 3 commits into from
Jan 24, 2022
Merged

[dashboard] show repo name when workspace creation fails #7302

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ccf8761
[dashboard] show repo name when workspace creation fails for repo not…
trumbitta Dec 17, 2021
41c4276
[dashboard] use utility component for repo owner in messages
trumbitta Dec 21, 2021
a93edc0
[dashboard] make a primary button of the only call-to-action
trumbitta Jan 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 19 additions & 13 deletions components/dashboard/src/start/CreateWorkspace.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { openAuthorizeWindow } from "../provider-utils";
import { SelectAccountPayload } from "@gitpod/gitpod-protocol/lib/auth";
import { SelectAccountModal } from "../settings/SelectAccountModal";
import { watchHeadlessLogs } from "../components/PrebuildLogs";
import CodeText from "../components/CodeText";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I had no idea we had this component. 😅 I agree with @trumbitta that a <code> element would be more semantic than a <span>.

Also, creating single-line React components is not the best way to share common style (because it adds unnecessary code and complexity, and it's too easy to forget about using the component where relevant). I think we should remove the CodeText component again, and instead apply our common CSS rules to the code element inside index.css.

However, this is off-topic here and can definitely be a follow-up issue / Pull Request. @trumbitta would you be interested in filing an issue for this? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had already created one :) might be worth it to chime in and add your perspective? #7555


const WorkspaceLogs = React.lazy(() => import('../components/WorkspaceLogs'));

Expand Down Expand Up @@ -233,9 +234,11 @@ function LimitReachedOutOfHours() {

function RepositoryNotFoundView(p: { error: StartWorkspaceError }) {
const [statusMessage, setStatusMessage] = useState<React.ReactNode>();
const { host, owner, repoName, userIsOwner, userScopes, lastUpdate } = p.error.data;
const repoFullName = (owner && repoName) ? `${owner}/${repoName}` : '';

useEffect(() => {
(async () => {
const { host, owner, repoName, userIsOwner, userScopes, lastUpdate } = p.error.data;
console.log('host', host);
console.log('owner', owner);
console.log('repoName', repoName);
Expand All @@ -248,8 +251,6 @@ function RepositoryNotFoundView(p: { error: StartWorkspaceError }) {
return;
}

const repoFullName = (owner && repoName) ? `${owner}/${repoName}` : '';

// TODO: this should be aware of already granted permissions
const missingScope = authProvider.host === 'github.com' ? 'repo' : 'read_repository';
const authorizeURL = gitpodHostUrl.withApi({
Expand All @@ -259,15 +260,15 @@ function RepositoryNotFoundView(p: { error: StartWorkspaceError }) {

if (!userScopes.includes(missingScope)) {
setStatusMessage(<div className="mt-2 flex flex-col space-y-8">
<p className="text-base text-gray-400 w-96">The repository '{`${repoFullName}`}' may be private. Please authorize Gitpod to access to private repositories.</p>
<a className="mx-auto" href={authorizeURL}><button className="secondary">Grant Access</button></a>
<p className="text-base text-gray-400 w-96">The repository may be private. Please authorize Gitpod to access to private repositories.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<p className="text-base text-gray-400 w-96">The repository may be private. Please authorize Gitpod to access to private repositories.</p>
<p className="text-base text-gray-400 w-96">The repository <code>${repoName}</code> may be private. Please authorize Gitpod to access to private repositories.</p>

Copy link
Contributor Author

@trumbitta trumbitta Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Care to elaborate on why?

I reckon that keeping it inside the error message makes it pass as "unimportant" and makes it also easily misread.
In the new proposed position the repo name is front and center, bigger, red: meaning that next time, the next user won't likely have the same hiccup @ghuntley lamented about in #7191 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a more informative way to display to the users, maybe!?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to mention that keeping the repo name also in the error message looked bad both to me and @iQQBot . See #7302 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Siddhant-K-code @trumbitta! Agree, keeping the repository out of the error message is better as this information is now anchored in a single place that also helps memory muscle. 💪

<a className="mx-auto" href={authorizeURL}><button>Grant Access</button></a>
Copy link
Contributor

@jankeromnes jankeromnes Jan 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Praise: Removing the secondary class from these buttons is such an awesome drive-by fix. 💯 Many thanks @trumbitta and @gtsiolis. ❤️

</div>);
return;
}

if (userIsOwner) {
setStatusMessage(<div className="mt-2 flex flex-col space-y-8">
<p className="text-base text-gray-400 w-96">The repository '{`${repoFullName}`}' is not found in your account.</p>
<p className="text-base text-gray-400 w-96">The repository was not found in your account.</p>
</div>);
return;
}
Expand All @@ -284,22 +285,27 @@ function RepositoryNotFoundView(p: { error: StartWorkspaceError }) {

if (!updatedRecently) {
setStatusMessage(<div className="mt-2 flex flex-col space-y-8">
<p className="text-base text-gray-400 w-96">Permission to access private repositories has been granted. If you are a member of '{owner}', please try to request access for Gitpod.</p>
<a className="mx-auto" href={authorizeURL}><button className="secondary">Request Access for Gitpod</button></a>
<p className="text-base text-gray-400 w-96">Permission to access private repositories has been granted. If you are a member of <CodeText>{owner}</CodeText>, please try to request access for Gitpod.</p>
<a className="mx-auto" href={authorizeURL}><button>Request Access for Gitpod</button></a>
</div>);
return;
}

setStatusMessage(<div className="mt-2 flex flex-col space-y-8">
<p className="text-base text-gray-400 w-96">Your access token was updated recently. Please try again if the repository exists and Gitpod was approved for '{owner}'.</p>
<a className="mx-auto" href={authorizeURL}><button className="secondary">Try Again</button></a>
<p className="text-base text-gray-400 w-96">Your access token was updated recently. Please try again if the repository exists and Gitpod was approved for <CodeText>{owner}</CodeText>.</p>
<a className="mx-auto" href={authorizeURL}><button>Try Again</button></a>
</div>);
})();
}, []);

return <StartPage phase={StartPhase.Checking} error={p.error}>
{statusMessage}
</StartPage>;
return (
<StartPage phase={StartPhase.Checking} error={p.error}>
<p className="text-base text-gray-400 mt-2">
<CodeText>{repoFullName}</CodeText>
</p>
{statusMessage}
</StartPage>
);
}

interface RunningPrebuildViewProps {
Expand Down