Skip to content

[dashboard] Misc style updates #3619

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

Merged
merged 1 commit into from
Mar 29, 2021
Merged

[dashboard] Misc style updates #3619

merged 1 commit into from
Mar 29, 2021

Conversation

gtsiolis
Copy link
Contributor

No description provided.

@gtsiolis gtsiolis force-pushed the gt/misc-style-updates branch from af9e396 to 84dffcb Compare March 27, 2021 20:01
@gtsiolis
Copy link
Contributor Author

gtsiolis commented Mar 29, 2021

/werft run

👍 started the job as gitpod-build-gt-misc-style-updates.12

@gtsiolis gtsiolis marked this pull request as ready for review March 29, 2021 10:00
Copy link
Contributor Author

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

This should fix in some small visual details. @svenefftinge could you review and take over the remaining task to deep link to All Workspaces. WDYT?

Two things to consider before merging:

  1. Deep link to All Workspaces from the empty Workspaces list.
  2. New Integration button placement.

<div className="text-center pb-6 text-gray-500">Prefix any git repository URL with gitpod.io/# or create a new workspace for a recently used project. <a className="text-gray-400 underline underline-thickness-thin underline-offset-small hover:text-gray-600" href="https://www.gitpod.io/docs/getting-started/">Learn more</a></div>
<span>
<button onClick={this.showStartWSModal} className="font-medium">New Workspace</button>
<button className="ml-2 font-medium border-gray-500 text-gray-500 bg-white hover:bg-gray-100 hover:bg-border-700 hover:text-gray-700">View All Workspaces</button>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue: Probably the most important change in this one. We'll need to implement the deep link to All Workspaces.

Copy link
Member

Choose a reason for hiding this comment

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

I have made it so that the button only occurs if there are actually any workspaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! ✔️

@@ -35,15 +35,15 @@

@layer components {
button {
@apply cursor-pointer px-4 py-2 my-auto bg-green-600 hover:bg-green-700 border-2 border-green-800 text-gray-100 text-sm font-medium rounded-md focus:outline-none focus:ring focus:border-blue-300;
@apply cursor-pointer px-4 py-2 my-auto bg-green-600 hover:bg-green-700 border-2 border-green-600 hover:border-green-800 hover:border-green-800 text-gray-100 text-sm font-medium rounded-md focus:outline-none focus:ring focus:border-blue-300 transition ease-in-out;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue: Need to make sure this doesn't override with changes in #3620

}

button:disabled {
@apply cursor-default;
}

input[type=text] {
@apply text-xs block w-56 text-sm text-gray-600 rounded-md border-2 border-gray-400 focus:border-gray-500 focus:bg-white focus:ring-0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅

<h3>Environment Variables</h3>
<h2 className="text-gray-500">Variables are used to store information like passwords.</h2>
</div>
{envVars.length !== 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thought: Not sure if there's a better more efficient way to check this here.

Copy link
Member

Choose a reason for hiding this comment

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

looks good

{providers.length !== 0
?
<div className="mt-3 flex mt-0">
<button onClick={() => setModal({ mode: "new" })} className="ml-2 font-medium">New Integration</button>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thought: This could look like weird. An alternative plan could be to a) introduce the table header (see Workspaces, Environment Variables) to add some visual balance or b) keep the New Integration button below the list.

@gtsiolis gtsiolis requested a review from svenefftinge March 29, 2021 10:03
@@ -45,7 +45,7 @@ export default function Account() {

<SettingsPage title='Account' subtitle='Manage account and git configuration.'>
Copy link
Contributor Author

@gtsiolis gtsiolis Mar 29, 2021

Choose a reason for hiding this comment

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

issue: We're still missing the whitespace on the right of this page after a recent change. See #3582 (comment)

@svenefftinge svenefftinge force-pushed the gt/misc-style-updates branch from 8edfea1 to de29078 Compare March 29, 2021 11:53
@gtsiolis gtsiolis added this to the March 2021 milestone Mar 29, 2021
@svenefftinge svenefftinge force-pushed the gt/misc-style-updates branch from de29078 to 3e1c766 Compare March 29, 2021 12:18
Copy link
Member

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

🍊

@gtsiolis
Copy link
Contributor Author

Good to merge?

@svenefftinge svenefftinge merged commit 4d670bc into main Mar 29, 2021
@svenefftinge svenefftinge deleted the gt/misc-style-updates branch March 29, 2021 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants