-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Improve start page when GitHub App not installed #7163
Conversation
This is still a WIP based on ongoing discussions. Just to make it easier to collect input from you @jldec @gtsiolis. |
Thanks @laushinka
from @gtsiolis
|
Haven't dived into the changes yet but just seeing the avatar, username, and provider URL in the dropdown element makes this page 10x better. 🤯 🔝 |
@jldec Thanks for the review!
What are the width differences?
For the top, is it (Thanks for teaching me this, @gtsiolis 👇🏼 😛)
|
I think what I'm seeing is a width difference between the app install layout and the repo-select layout. |
@jldec Noted. I'll check with @gtsiolis on which card in Figma I should follow, so I can inspect the attributes. And if everything else looks fine, I will wait for George's input so I'll tackle the design improvements altogether 🙏🏽 |
Go markdown tables[1]! Looking at this now! 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @laushinka! 🌟
Left some comments below regarding UX changes. Current changes already improve this area of the product. Feel free to ingore any of these and merge for this iteration. ➿
Re-posting from https://github.com/gitpod-io/gitpod/pull/7163/files#issuecomment-990959643:
Haven't dived into the changes yet but just seeing the avatar, username, and provider URL in the dropdown element makes this page 10x better. 🤯 🔝
Re-apporving to unblock merging but holding in case you'd resolve any of the comments below.
/hold
LGTM label has been added. Git tree hash: 5fda46f4ce600dfb6129bb83e92e1b1f1ae46bdf
|
bf91b88
to
504fee1
Compare
/werft run 👍 started the job as gitpod-build-laushinka-start-page-6875.13 |
/werft run 👍 started the job as gitpod-build-laushinka-start-page-6875.14 |
Let's keep this out of the scope for now. Only posted this here in case this PR made any changes to that step (page), too. ➿ |
)} | ||
<div> | ||
<div className="text-gray-500 text-center w-96 mx-8"> | ||
Repository not found? <a href="javascript:void(0)" onClick={e => reconfigure()} className="text-gray-400 underline underline-thickness-thin underline-offset-small hover:text-gray-600">Reconfigure</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This text should only appear when with the repositorylist is showing. It is not required when we prompt for app authorization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And only for GitHub, right? Or all providers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - GitHub only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks @laushinka! As mentioned on Slack, this is so much nicer now 😍 ✨
Also, the code looks great to me. 🎯 Many thanks for caring about readability. 💯
I've added a few thoughts, but they're entirely optional or out of scope, so please feel free to read them later. 🌴
Approving because the code can be merged as is and works flawlessly.
/lgtm
LGTM label has been added. Git tree hash: a2decbc7e42bfaf6c5af7e1f77989fc5f671d4c9
|
10e80fe
to
7cbfd6d
Compare
7cbfd6d
to
4518f2a
Compare
@jankeromnes Pushed improvements for the code 🙌🏽 |
Currently the preview env is broken as I rebased (probably should've held off on that) while we have an ongoing problem. |
4518f2a
to
cee6ae0
Compare
Preview env all good now. |
/lgtm |
LGTM label has been added. Git tree hash: bc4371704dd9c17a248415caf41c914d484c9323
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gtsiolis, jankeromnes, jldec Associated issue: #6875 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/unhold |
Description
Should do as described here:
Related Issue(s)
Resolves #6875
How to test
Release Notes
Documentation