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.

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

include <kbd> in running-text support #2475

Merged
merged 4 commits into from
May 8, 2018
Merged

include <kbd> in running-text support #2475

merged 4 commits into from
May 8, 2018

Conversation

giladgray
Copy link
Contributor

@giladgray giladgray commented May 7, 2018

another element that should not require a CSS class

@giladgray giladgray requested a review from llorca May 7, 2018 22:14
box-shadow: $pt-elevation-shadow-2;
background: $white;
min-width: $kbd-key-size;
height: $kbd-key-size;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@llorca this is a good time to fix #2453. what changes should I make? (or push them yourself)

Copy link
Contributor

Choose a reason for hiding this comment

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

@giladgray <kbd>s should be

  • same height as small buttons
  • inline-flex
  • make sure icons and text are vertically aligned

@blueprint-bot
Copy link

include `` in running-text support

Preview: documentation | landing | table

@blueprint-bot
Copy link

update docs

Preview: documentation | landing | table

@@ -281,6 +285,38 @@ Styleguide preformatted
@extend %code-block;
}

%keyboard {
Copy link
Contributor

Choose a reason for hiding this comment

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

remind me why we do this again? I'd have taken this opportunity to kill this pattern

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bro i un-killed it for this opportunity! https://github.com/palantir/blueprint/pull/2366/files#diff-63be20ab91cfbb20b47f78f5a125aea1R165

this is a perfect use case for placeholders: one set of styles that we want tied to two selectors - .X and .pt-running-text x

Copy link
Contributor

@llorca llorca left a comment

Choose a reason for hiding this comment

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

Fixing #2453 right now

@blueprint-bot
Copy link

fix kbd style/layout

Preview: documentation | landing | table

@llorca
Copy link
Contributor

llorca commented May 8, 2018

@giladgray review your own PR 🙏

display: inline-block;
border-radius: $pt-border-radius - 1;
box-shadow: $pt-elevation-shadow-2;
$kbd-key-size: $pt-button-height-small;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

delete this variable alias, re-use the existing one

$kbd-key-size: $pt-button-height-small;
display: inline-flex;
align-items: center;
justify-content: center;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not necessary - element is never wider than content

background: $white;
min-width: $kbd-key-size;
height: $kbd-key-size;
padding: ($pt-grid-size * 0.3) ($pt-grid-size / 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use 3px 6px ($pt-border-radius ($pt-border-radius * 2)) and remove modifier-key overrides below

color: $pt-text-color-muted;
font-family: inherit;
font-size: $pt-font-size-small;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

needs vertical-align

image

@giladgray
Copy link
Contributor Author

@llorca please re-review and be sure to check hotkeys dialog (?)

@blueprint-bot
Copy link

refactor keyboard & hotkeys css

Preview: documentation | landing | table

Copy link
Contributor

@llorca llorca left a comment

Choose a reason for hiding this comment

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

looking great ✨

@giladgray giladgray merged commit 7311243 into develop May 8, 2018
@giladgray giladgray deleted the gg/keyboard branch May 8, 2018 21:19
This was referenced May 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants