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

[UI Framework] [ K7] Fix code block font-family and annotate reset #13219

Merged
merged 2 commits into from
Jul 31, 2017
Merged

[UI Framework] [ K7] Fix code block font-family and annotate reset #13219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40f3cad
Fix broken code block font family.
cjcenizal Jul 31, 2017
8dc6d75
Fix font family of KeyPadMenuItem.
cjcenizal Jul 31, 2017
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
26 changes: 23 additions & 3 deletions ui_framework/dist/ui_framework.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@
*, *:before, *:after {
box-sizing: border-box; }

/**
* 1. Inheriting the font will allow some browser defaults to take effect, e.g. Chrome applies
* `font: 11px system-ui` to the button element. We can't hardcode the font-family here because
* that will disrupt components which rely upon a different inherited font-family, e.g. code
* blocks.
*/
html, body, div, span, applet, object, iframe,
h1, h2, h3, h4, h5, h6, p, blockquote, pre,
a, abbr, acronym, address, big, cite, code,
Expand All @@ -23,7 +29,9 @@ time, mark, audio, video {
border: 0;
font-size: 100%;
font: inherit;
font-family: "Roboto", Helvetica, Arial, sans-serif;
/* 1 */
font-family: inherit;
/* 1 */
vertical-align: baseline; }

/* HTML5 display-role reset for older browsers */
Expand Down Expand Up @@ -524,6 +532,12 @@ table {
width: 40px;
height: 40px; }

/**
* Dial menu is a phone like dial comprised of an svg icon and a title.
*/
/**
* 1. Default to grid of 3.
*/
.kuiKeyPadMenu {
display: -webkit-box;
display: -webkit-flex;
Expand All @@ -537,8 +551,12 @@ table {
-webkit-flex-wrap: wrap;
-ms-flex-wrap: wrap;
flex-wrap: wrap;
width: 288px; }
width: 288px;
/* 1 */ }

/**
* 1. If this class is applied to a button, we need to override the Chrome default font.
*/
.kuiKeyPadMenuItem {
display: block;
padding: 16px;
Expand All @@ -547,7 +565,9 @@ table {
color: #666;
border: 1px solid #D9D9D9;
border-color: transparent;
border-radius: 4px; }
border-radius: 4px;
font-family: "Roboto", Helvetica, Arial, sans-serif;
/* 1 */ }
.kuiKeyPadMenuItem:hover, .kuiKeyPadMenuItem:focus {
border-color: #D9D9D9; }
.kuiKeyPadMenuItem:hover .kuiKeyPadMenuItem__icon, .kuiKeyPadMenuItem:focus .kuiKeyPadMenuItem__icon {
Expand Down
13 changes: 11 additions & 2 deletions ui_framework/src/components/key_pad_menu/_key_pad_menu.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
// Dial menu is a phone like dial comprised of an svg icon and a title.
/**
* Dial menu is a phone like dial comprised of an svg icon and a title.
*/

/**
* 1. Default to grid of 3.
*/
.kuiKeyPadMenu {
display: flex;
flex-direction: row;
flex-wrap: wrap;
width: $kuiKeyPadMenuSize * 3; // Default to grid of 3
width: $kuiKeyPadMenuSize * 3; /* 1 */
}

/**
* 1. If this class is applied to a button, we need to override the Chrome default font.
*/
.kuiKeyPadMenuItem {
display: block;
padding: $kuiSize;
Expand All @@ -16,6 +24,7 @@
border: $kuiBorderThin;
border-color: transparent;
border-radius: $kuiBorderRadius;
font-family: $kuiFontFamily; /* 1 */

&:hover, &:focus {
border-color: $kuiBorderColor;
Expand Down
10 changes: 8 additions & 2 deletions ui_framework/src/global_styling/reset/_reset.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@
box-sizing: border-box;
}

/**
* 1. Inheriting the font will allow some browser defaults to take effect, e.g. Chrome applies
* `font: 11px system-ui` to the button element. We can't hardcode the font-family here because
* that will disrupt components which rely upon a different inherited font-family, e.g. code
* blocks.
*/
html, body, div, span, applet, object, iframe,
h1, h2, h3, h4, h5, h6, p, blockquote, pre,
a, abbr, acronym, address, big, cite, code,
Expand All @@ -24,8 +30,8 @@ time, mark, audio, video {
padding: 0;
border: 0;
font-size: 100%;
font: inherit;
font-family: $kuiFontFamily;
font: inherit; /* 1 */
font-family: inherit; /* 1 */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snide Here's a good example of one of the benefits of following the style guide's commenting recommendation. The comment applies to multiple properties, and we can use the citation number to communicate that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I find it hard to read. You have to scan multiple lines to see the comment. I don't see this style anywhere in larger frameworks. They all use single line comments either above or next to the properties in use.

I don't see much use in adding a key for CSS blocks. At most you're looking at under 10 defined properties. Rather just see it in use against the content where it's being defined.

It's a small place where I think their recommendation is off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I hear you. What's your solution for where a comment applies to multiple lines? And for where multiple comments apply to a single line?

Copy link
Contributor

Choose a reason for hiding this comment

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

For two, where you have a long block of text I'd just separate it out.

.cssThing {
  color: white;
  background: blue;
  // Zoom and outline are set to reset Internet Explorer
  // and Firefox. Blah blah, long comment spans multiple lines.
  zoom: 1;
  outline: none;
}

For 90% of comments though, the explanation is pretty short and on one item. I can be swayed here if you want it uniform and above. I just find this works most of the time.

.cssThing {
  color: blue;
  background: red;
  zoom: 1; // Fix for IE 11.
}

The other benefit is this means that comments above the selector mean they are actually commenting the selector itself.

// This is used when the popover does this weird thing.
.cssThing--modifier {
}

vertical-align: baseline;
}

Expand Down