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

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

Merged
merged 2 commits into from
Jul 31, 2017

Conversation

cjcenizal
Copy link
Contributor

No description provided.

@cjcenizal cjcenizal added the Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. label Jul 31, 2017
@cjcenizal cjcenizal requested a review from snide July 31, 2017 15:06
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 {
}

@cjcenizal cjcenizal merged commit 3d04798 into elastic:k7-ui-framework Jul 31, 2017
@cjcenizal cjcenizal deleted the k7/code-font branch July 31, 2017 22:12
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Aug 16, 2017
…lastic#13219)

* Fix broken code block font family.
* Fix font family of KeyPadMenuItem.
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Aug 26, 2017
…lastic#13219)

* Fix broken code block font family.
* Fix font family of KeyPadMenuItem.
cjcenizal added a commit that referenced this pull request Sep 19, 2017
…13219)

* Fix broken code block font family.
* Fix font family of KeyPadMenuItem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants