Skip to content

[dashboard] button styling #3620

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] button styling #3620

merged 1 commit into from
Mar 29, 2021

Conversation

svenefftinge
Copy link
Member

@gtsiolis as discussed the buttons without border.
Besides that, I tried kumquat (WDYT?).
I also tried a height of 40px but that makes the buttons very big (see below). So I rolled back to 36px.

Screenshot 2021-03-27 at 22 26 24

@svenefftinge svenefftinge requested a review from gtsiolis March 27, 2021 21:27
Copy link
Contributor

@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.

Go kumquat! 🍊

@@ -35,23 +35,29 @@

@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-gitpod-kumquat-dark hover:bg-gitpod-kumquat-darker text-white text-sm font-medium rounded-md focus:outline-none focus:ring;
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Strong vote for not changing the primary color to kumquat although this looks yummy! Orange (~kumquat) color usually indicates a warning or something is pending. But we could try this and revert later. 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

I understand. I feel it makes the branding stronger and with that, it doesn't feel so much signaling but fruity.

@@ -24,6 +24,7 @@ module.exports = {
'gitpod-kumquat-light': '#FFE4BC',
'gitpod-kumquat': '#FFB45B',
'gitpod-kumquat-dark': '#FF8A00',
'gitpod-kumquat-darker': '#f28300',
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Oh, way down we go! 😸

Copy link
Member Author

Choose a reason for hiding this comment

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

just to give it a little hover effect. the normal kumquat felt too bright for the buttons.😇

@@ -35,23 +35,29 @@

@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-gitpod-kumquat-dark hover:bg-gitpod-kumquat-darker text-white text-sm font-medium rounded-md focus:outline-none focus:ring;
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Height looks good also with 36px although this will break the pattern of the 8px grid. Negiotiable! Let's do it. 💭

Copy link
Member Author

Choose a reason for hiding this comment

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

we could reduce it to 32px but that might be too small.

@@ -67,7 +67,7 @@ export default function Account() {
</div>
<h3 className="mt-12">Delete Account</h3>
<p className="text-base text-gray-500 pb-4">This action will remove all the data associated with your account in Gitpod.</p>
<button className="border-red-600 text-red-600 bg-white hover:border-red-800 hover:text-red-800" onClick={() => setModal(true)}>Delete Account</button>
<button className="warn" onClick={() => setModal(true)}>Delete Account</button>
Copy link
Contributor

@gtsiolis gtsiolis Mar 27, 2021

Choose a reason for hiding this comment

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

thought: More button variants will probaly emerge in the long run but I think this would fit better as a danger button, not a warning button. It should be fine as warn as for the time being we need this once in Account. Danger buttons can describe a destructive action that can't be undone like deleting something.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, danger sounds better. ☢️

@@ -39,7 +39,7 @@ export default function Account() {
</div>
<div className="flex justify-end mt-6">
<button className="text-gray-900 border-white bg-white hover:border-gray-200" onClick={close}>Cancel</button>
<button className={"ml-2 border-red-900 bg-red-500 disabled:opacity-50" + (typedEmail !== primaryEmail ? '' : ' hover:bg-red-700')} onClick={deleteAccount} disabled={typedEmail !== primaryEmail}>Delete Account</button>
<button className={"ml-2 warn disabled:opacity-50" + (typedEmail !== primaryEmail ? ' hover:bg-gray-100' : '')} onClick={deleteAccount} disabled={typedEmail !== primaryEmail}>Delete Account</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: This should become a primary button using the danger variant, not using the outlined version, as this it the single primary action (CTA) in this screen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, so we would have primary danger (white on red bg) and secondary danger (red on white with red border)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly! Also, tertiary danger could be needed in the future. See borderless Cancel button (tertiary default) on some modals. 🎯

@@ -35,23 +35,29 @@

@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;
Copy link
Contributor

@gtsiolis gtsiolis Mar 27, 2021

Choose a reason for hiding this comment

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

issue: We're going to need the border-2 for secondary buttons, see delete button in Account. ❗

@svenefftinge
Copy link
Member Author

svenefftinge commented Mar 28, 2021

/werft run

👍 started the job as gitpod-build-se-button-styles.1

@svenefftinge svenefftinge marked this pull request as ready for review March 28, 2021 20:07
@svenefftinge
Copy link
Member Author

svenefftinge commented Mar 28, 2021

I removed all the inline styles from the buttons and generalized the styles in the central css to make them all aligned.
When the height is to 32px (to make it fit in the 8px grid) it looks like this.

Screenshot 2021-03-28 at 22 07 36

With 36px it looks like this:

Screenshot 2021-03-28 at 22 17 05

@svenefftinge
Copy link
Member Author

svenefftinge commented Mar 29, 2021

/werft run

👍 started the job as gitpod-build-se-button-styles.3

@svenefftinge svenefftinge force-pushed the se/button-styles branch 2 times, most recently from 75d8c5a to 1192c4e Compare March 29, 2021 08:19
Copy link
Contributor

@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.

Overall looks good! Left some minor comments.

This is minor but feel free to also remove the obsolete screenshots (orange buttons) from the PR description to avoid confusion in the future.

@@ -35,23 +35,34 @@

@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 text-gray-100 text-sm font-medium rounded-md focus:outline-none focus:ring;
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Phew! That was close! Thanks for going back to green here! 😅

@@ -13,10 +13,10 @@ export interface SelectableCardProps {
}

function SelectableCard(props: SelectableCardProps) {
return <div className={`rounded-xl px-4 py-3 flex flex-col cursor-pointer group border-2 ${props.selected ? 'border-green-600' : 'border-gray-300 hover:border-gray-400'} ${props.className || ''}`} onClick={props.onClick}>
return <div className={`rounded-xl px-4 py-3 flex flex-col cursor-pointer group border ${props.selected ? 'border-green-500' : 'border-gray-300 hover:border-gray-400'} ${props.className || ''}`} onClick={props.onClick}>
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: 1px border here looks so thin and not quite clear what's selected, right? WDYT of using border-2 for cards?

@@ -613,7 +613,7 @@ function GitIntegrationModal(props: ({
)}
</div>
<div className="flex justify-end mt-6">
<button className="disabled:opacity-50" onClick={() => validate() && activate()} disabled={!!validationError || busy}>Activate Integration</button>
<button onClick={() => validate() && activate()} disabled={!!validationError || busy}>Activate Integration</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: Could we also remove the hover state when this is disabled? Same for updating git permissions.

@@ -150,7 +150,7 @@ export function WorkspaceEntry({ desc, model }: { desc: WorkspaceInfo, model: Wo
</div>
</div>
<div className="flex justify-end mt-5">
<button className="cursor-pointer px-3 py-2 text-white text-sm rounded-md border-2 border-red-800 bg-red-600 hover:bg-red-800"
<button className="danger"
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Feels good to see this becoming simpler. I was expecting to see this in the next iteration but now sounds also good. ☺️

@svenefftinge svenefftinge merged commit 9c4971d into main Mar 29, 2021
@svenefftinge svenefftinge deleted the se/button-styles branch March 29, 2021 10:32
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