Skip to content
This repository was archived by the owner on Dec 11, 2021. It is now read-only.

Initial base typography #57

Conversation

MichaelArestad
Copy link
Contributor

Still in progress. Will keep pecking away at it.

#16

@@ -0,0 +1,158 @@
<!doctype html>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to include this as part of the commit? A page like this would probably count as being part of the "Pattern Library," and probably shouldn't be kept in the home directory

background-repeat: repeat-x;
background-size: 2px 2px;
background-position: 0 em( 22px );
outline: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Outlines should only be modified using :focus. Use of outline: none or outline: 0 should be limited to :focus rules. (outline-none)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? I'm not opposed to the change, but I'm curious as to the reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

Its a pretty big accessibility issue because the outline property is what makes the focus highlight in most browsers. So if you set it to 0 on all a elements now there is no focus indication on any links on the page. Here is a good explination in detail if your interested. https://github.com/CSSLint/csslint/wiki/Disallow-outline%3Anone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm aware, but that's a non-issue if there are other visual focus styles in place, which there are.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only focus style i see here is setting a background size, position, repeat and a gradient, which does not really do anything besides make the underline slightly bolder unless i'm missing something? I consider that a pretty big accessibility issue.

This can be seen tabbing through your demo too. I don't think think people would even notice the focus indicator here in general.
http://view.css-chassis.com/typography/typography.html
Compare this to the focus style on a normal link without any styles applied.
http://jsbin.com/waxeyo/1

I personally am against changing the focus outline at all from what the native is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to give some context on why i don't think we should alter native focus outlines. We used to have non native focus styles in jQuery UI and had TONS of complaints, until we removed it.
Based of the accessibility concerns involved with focus indication i think its best to leave the style users are familiar with on their particular device.

I do however also think we should provide a visual focus style for certain situations like showing a focus indication where native can't. An example is toolbars or accessible styled radio buttons and checkboxes. This situation came up recently and after consulting with several accessibility experts this is what we came up with.
http://view.jqueryui.com/button-classes/demos/checkboxradio/default.html
You can see tabbing a visual focus outline ( that may depending on your browser / os actually look native )

The problem is the visual button is a label. Labels can't receive focus they transfer to the underlying input which is accessibly hidden. So this means there is no focus outline visible. So we created a class that adds a visual focus indicator to the label. Problem here is the visual focus style and the regular focus style can be inconsistent on some platforms.

We add our own focus style everywhere for this same reason in jQuery mobile and this solves the issue of visual inconsistency however not the issue of user complaints ( or my own person desire to keep things as simple as possible for end users, navigating through a page via keyboard, by keeping important visual indicators native, as the way people are familiar with, in style ).

@arschmitz
Copy link
Contributor

Now that there is an html file we need to add the html task to add html linting. It's already in package.json

@sfrisk
Copy link
Contributor

sfrisk commented Mar 17, 2015

Not to mention we need to rework the HTML style guide.

On Tue, Mar 17, 2015 at 11:55 AM, Alexander Schmitz <
[email protected]> wrote:

Now that there is an html file we need to add the html task to add html
linting. It's already in package.json


Reply to this email directly or view it on GitHub
#57 (comment).

Sarah Frisk
sarahfrisk.com

Twitter: twitter.com/SarahFrisk
LinkedIn: linkedin.com/in/sarahfrisk
Github: github.com/sfrisk

@arschmitz arschmitz closed this in 206014c Apr 7, 2015
@arschmitz
Copy link
Contributor

oops wrong number reopening

@arschmitz arschmitz reopened this Apr 7, 2015
@arschmitz
Copy link
Contributor

I pushed the latest update on this to view.css-chassis.com

<meta name="description" content="Typography skeleton for styling">
<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="stylesheet" href="../dist/css/chassis.css">
<link href='http://fonts.googleapis.com/css?family=Source+Sans+Pro:400,600,700,400italic,700italic' rel='stylesheet' type='text/css'>
Copy link
Contributor

Choose a reason for hiding this comment

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

single quotes?

Copy link
Contributor

Choose a reason for hiding this comment

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

no what I meant is that this tag uses single quotes and all the rest doubles. We use double quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha! Will change.

@MichaelArestad MichaelArestad force-pushed the add/typographic-styles branch from e1e7ae1 to 76bfa64 Compare April 9, 2015 23:47
@MichaelArestad
Copy link
Contributor Author

Updated a few things. It's a good base that's ready to merge IMO. We can then iterate, add colors, etc, in other PRs.

@MichaelArestad
Copy link
Contributor Author

Failed check because of the hyphen in the attribute name, BTW.

@@ -59,6 +59,7 @@
"grunt-csscomb": "3.0.0",
"grunt-git-authors": "2.0.0",
"grunt-html": "1.6.0",
"grunt-htmllint": "^0.2.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

We use firm versions to do our best to avoid weird issues with different people having different versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. Will change.

@arschmitz
Copy link
Contributor

I'm wondering if we really want the x-ua-compatible meta since this forces the browser mode. while on production sites i would always use this, the demos are as much for our own visual testing, as they are for demos, i think i would prefer to leave it out.

Also just as a side note, we have never included this on the ui or mobile demos and there has never been an issue. We do of course keep it on our production WP sites though.

@arschmitz
Copy link
Contributor

Can you also add demos.css to the csslint settings?

@MichaelArestad
Copy link
Contributor Author

@arschmitz Changes made:

  • added hard version.
  • removed x-ua-compatible
  • added demos.css to csslint

<html>
<head>
<meta charset="utf-8">
<meta content="ie=edge">
Copy link
Contributor

Choose a reason for hiding this comment

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

If we remove the x-ua-compatible there's no deal with including this anymore.

<meta name="viewport" content="width=device-width, initial-scale=1">
<link rel="stylesheet" href="../dist/css/chassis.css">
<link rel="stylesheet" href="demos.css">
<link href="http://fonts.googleapis.com/css?family=Source+Sans+Pro:400,600,700,400italic,700italic" rel="stylesheet" type="text/css">
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the type="text/css"

@arthurvr arthurvr mentioned this pull request Apr 21, 2015
@sfrisk
Copy link
Contributor

sfrisk commented May 1, 2015

Any more updates on this?

@MichaelArestad
Copy link
Contributor Author

@sfrisk Good to go.

@@ -0,0 +1,26 @@
// ==========================================================================
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry i didn't notice the before but the style guide mandates /* comment */ style comments since these are the only valid style in normal css. @MichaelArestad can you please switch all the comments here to use this style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is using multiline comments the plan for the Sass guide?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realize this style has slipped in already in other files, we should switch these to match the style guide in a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

@MichaelArestad well its not necessarily multi line it the only valid style in normal css. I personally don't think its good practice to use different comment styles in different files so we should stick to valid css comments i don't think using // has any benefit over /* */ other then not typing 2 chars which git stripped out in real build anyway i think clarity and consistency should prevail here. @sfrisk what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are minor benefits which are situational. No single line comments end up in output CSS files whereas multiline comments do depending on output style. This is handy for when you want to comment things like functions without it ending up in the output code or when you are generating a theme file and want to curate which comments end up in the compiled CSS file.

Copy link
Contributor

Choose a reason for hiding this comment

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

No single line comments end up in output CSS files whereas multiline comments do depending on output style

So no comments at all end up in minified files which is really what matter. We also use the "Expanded" style which outputs both single and multi line comments so there is no difference here either.

This is handy for when you want to comment things like functions without it ending up in the output code or when you are generating a theme file and want to curate which comments end up in the compiled CSS file.

This would be true if our output style accommodated this however it does not. Only the compressed output style accommodates this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expanded does not output single line comments. No style does.

Compressed, expanded, and nested support outputting multiline comments (without doing a forced comment /*! */).

Copy link
Contributor

Choose a reason for hiding this comment

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

@MichaelArestad Your right sorry i looked this up and misread. You are correct here.

So in this case there is basically one major case that makes sense to use single line comments which is where we have a comment that would end up with no context and make no sense if it ended up in an uncompressed output file. This would likely be the case for most functions. Most other places should probably stick to multiline i think still. Since we have both css and scss files in this repo i would like to try to keep things as clear and consistent as possible. Also i think anything that deserved a comment in the scss file should still have one in the uncompressed development build file.

@MichaelArestad how does that sound to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. Probably functions and comments that are implementation instructions for mixins as well. Will change on another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably functions and comments that are implementation instructions for mixins as well.

I think a general rule of "will this lose context when its in the built file" could apply for what should use single line.

On a slight aside these sorts of comments in these 2 cases should probably eventually move to the documentation ( once such a thing exists ) but for now yes i agree.

@arschmitz arschmitz closed this in 64a51ac May 5, 2015
@MichaelArestad MichaelArestad deleted the add/typographic-styles branch May 7, 2015 03:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants