eslint / archive-website

The ESLint website
https://eslint.org
MIT License
96 stars 244 forks source link

Hard to separate working blocks from failing ones #108

Closed nirazul closed 8 years ago

nirazul commented 9 years ago

I know the docs are autogenerated. But maybe there's a way to do that:

Currently, every block is formatted the same way. It would be much clearer to use some sort of color coding to separate the "good" examples from the ones that fail tests. A good example for color coding is found on bootstrap.

Would be looking like that: rules

IanVS commented 9 years ago

I agree that it would be good to put some thought into UI/UX for this. I'm not sure if colors are the best way (considering r/g colorblind folks), but perhaps it could be used in conjunction with a larger redesign at some point? I'd love to see some consistency between how options are explained and examples are given. I just don't know how that design should look.

nirazul commented 9 years ago

Colors were just the most obvious way I came up with to solve the problem. I agree that this idea is most useful in conjunction with other measures. Currently, we are all treated as colorblinds (sort of). If you want to tackle it at a more general level, perhaps a table is a better way to display those qualities:

Rule: yoda default: "never"

Configuration validates fails
"yoda": [2, "never"] if (color === "red") {} if ("red" === color) {}

This would also solve the problem that much space is wasted on most screen sizes due to full-width code blocks.

nzakas commented 9 years ago

The problem becomes how to generate such changes automatically. It's unlikely we will get a volunteer to reformat every for by hand, so what can be done such automation?

btmills commented 9 years ago

If we can find a solution for this that will also solve the remaining blocker in my mind on eslint/eslint#2271 (how do we lint valid examples but only check invalid examples for syntax?), then we could solve this at the same time we (likely meaning "I") go through and use configuration comments (related discussion eslint/eslint#3208) for all the examples.

IanVS commented 9 years ago

Is it just a matter of wrapping code blocks with a div of an appropriate class? I don't know how the file loader / documentation linting works, but that could potentially be used at least for styling.

IanVS commented 9 years ago

And @btmills I would be happy to change docs to use configuration comments. Your mind/time is better utilized on less tedious activities.

ilyavolodin commented 9 years ago

I don't think it's a matter of wrapping the code in a div. We write everything in MD format. And code blocks are converted into <pre> tags by jekyll. I don't actually know how we can achieve valid vs. invalid styles with .md

IanVS commented 9 years ago
<div class="passing">

// md code block here

</div>

Is valid markdown, and the background can be styled with:

.passing pre {
    background:green;
}

I've tested it out in my blog (which also uses markdown) and it worked, and can try it on the this repo tonight.

nirazul commented 9 years ago

@IanVS If your example turns out to be working, would you agree with my idea to use tables? Basically, It'd be the same approach:

<div class="yep-nope-table">

// md table here

</div>
// css:
.yep-nope-table td:first-child { /* config block */ }
.yep-nope-table td:first-child + td { /* yep block */ }
.yep-nope-table td:first-child + td + td { /* nope block */ }
IanVS commented 9 years ago

I can try tables as an experiment, but in my experience markdown tables are kind of a pain to work with. I do think in general the result might look nicer and be clearer, but we could run into the issue of wrapping lines, which is always ugly in code examples. Also, changing to tables would require much more drastic changes to all of the rule docs. I'll try it out though.

nzakas commented 9 years ago

Just to reiterate: anything manual is not a good solution. These are manual solutions which mean we are going to have to manually convert everything and also manually enforce their usage on pull requests. In both cases, this is a lot of work, so I'm skeptical this is a good direction.

btmills commented 9 years ago

What if the examples in the docs were all live? If the demo could be made into something reusable, then invalid examples would have the actual error messages right there.

nzakas commented 9 years ago

And how would you automate that?

btmills commented 9 years ago

For illustration, I'll borrow an example from eqeqeq:

The following patterns are considered okay and do not cause warnings:

/* eslint eqeqeq: [2, "smart"] */
typeof foo == 'undefined'
'hello' != 'world'
0 == 0
true == true
foo == null

The following patterns are considered warnings with "smart":

/* eslint eqeqeq: [1, "smart"] */
// comparing two variables requires ===
a == b

// only one side is a literal
foo == true
bananas != 1

// comparing to undefined requires ===
value == undefined

npm test runs the linter on the docs using the Markdown preprocessor. Makefile.js will fail the build on errors (parse errors in valid and invalid examples and lint errors in the valid examples), and it will suppress warnings in the docs (from lint warnings in the invalid examples). This reduces our manual workload on PRs from manually checking and verifying examples for both syntax and linting down to making sure examples have a configuration comment that enables the rule.

The live demo part is hazier, as I don't know how the page build process works. @xjamundx is there (or could there be, with the work you're doing) a way to load a demo on code blocks within the page?

nzakas commented 9 years ago

Interesting. And how do we update the already existing docs?

IanVS commented 9 years ago

I've volunteered to update them.

nzakas commented 9 years ago

Cool :)

IanVS commented 9 years ago

I don't think it's a matter of wrapping the code in a div.

You're right, this doesn't work in redcarpet, Jekyll's markdown processor.

pedrottimark commented 8 years ago

@Nirazul @IanVS @nzakas @btmills @ilyavolodin

Can you think of any additional objectives which are relevant to this issue?

What do you think about the following pattern for the sentence preceding an example of code?

  1. The sentence begins with Examples of.
  2. Adjectives incorrect and correct decribe the code to avoid confusion with rule configuration error levels: warning or error.
  3. Omit any verb to avoid the passive voice: are considered.
  4. Make the rule configuration explicit:
    • Examples of incorrect code if you turn on the rule:
    • Examples of incorrect code for the default "always" option:
    • Examples of incorrect code for the "never" option:
    • Examples of incorrect code for the {"builtinGlobals": true} option:
    • Examples of incorrect code for the browser environment and the {"builtinGlobals": true} option:
    • Examples of incorrect code for the {"anonymous": "always", "named": "never"} option:
    • Examples of incorrect code for the {"anonymous": "never", "named": "always"} option:

Because the sentence includes configuration, what do you think if we omit punctuation in h3 headings for options to make a potential table of contents easier to scan?

Rule Details
Options
  always
  never
  anonymous
  named
When Not To Use It
Related Rules
Version
Resources

Considering the examples (in several following comments) which contrast baseline grammar and formatting with suggested changes, how well do you think the suggestions support the objectives?

ESLint members: and how realistic do you think it is for future editors of rule pages (including first-timers) to follow the suggested pattern (to a reasonable extent) after the initial rewriting, which I am willing to do?

pedrottimark commented 8 years ago

Rule has no options: no-console

Baseline:

no-console-baseline

Suggested:

no-console-suggested
pedrottimark commented 8 years ago

The default value of an option: consistent-this

Baseline:

consistent-this-default-baseline

Suggested:

consistent-this-default-suggested
pedrottimark commented 8 years ago

A second set of examples for an option: consistent-this

Baseline:

consistent-this-when-baseline

Suggested:

consistent-this-when-suggested
pedrottimark commented 8 years ago

Another value of an option: space-before-function-paren

Baseline:

space-before-other-baseline

Suggested:

space-before-other-suggested
pedrottimark commented 8 years ago

An object value of an option: space-before-function-paren

Baseline:

space-before-object-baseline

Suggested:

space-before-object-suggested
ilyavolodin commented 8 years ago

@pedrottimark I like it. I never cared much for "patterns are considered problems" I also like green/red backgrounds too. Maybe we can combine those two?

IanVS commented 8 years ago

I think the green/red problem is a technical one. I haven't found a way to accomplish it, but would love it if we could do at least some subtle shading differences.

pedrottimark commented 8 years ago

Let’s take a background distinction as part of the solution and trust that we will find a way. Just saw some resources for accessible color contrast ratios, so will make some example pictures to discuss.

In addition to distinguishing incorrect from correct, readers can quickly distinguish these examples from any general examples of code or configurations.

A question which I forgot to ask: If only some lines in a code example require es6 environment, what do you think about separating them? Compare the following to the preceding picture:

space-before-function-paren-suggested-es6
ilyavolodin commented 8 years ago

I think in the scope of a larger page, having a bunch of smaller wells might be slightly disconcerting. We should also think about possible future were we will support ES7 as well and possibly TypeScript/Flow. That might be just too many wells to deal with.

pedrottimark commented 8 years ago

Good point that we need to limit the total visual complexity.

ilyavolodin commented 8 years ago

I'll try to do kramdown upgrade this week, that might bring syntax highlighting, but I'm not 100% sure.

IanVS commented 8 years ago

@ilyavolodin any luck with Kramdown?

ilyavolodin commented 8 years ago

I tried it locally, and found two major issues. Code fences were not working and H1 headers were missing on rule docs. Code fences just required config switch, but H1 headers require modification of our build script. We do not leave empty line before H1s, so they don't render correctly. I'm not sure if there are any more issues, I've been pretty busy the last few days and haven't had the chance to look into it more.

pedrottimark commented 8 years ago

The following comments have pictures for your feedback about how background color could help people quickly see whether code is an incorrect, correct, or general example.

After the example sentences become consistent, a possible solution to the technical problem is replace rules in the doc.html layout file:

replace: '<p>Examples of <strong>incorrect</strong> code', '<p class="incorrect">Examples of <strong>incorrect</strong> code'

replace: '<p>Examples of <strong>correct</strong> code', '<p class="correct">Examples of <strong>incorrect</strong> code'

corresponding to CSS rules:

p.incorrect + div > pre { background-color: … } p.correct + div > pre { background-color: … }

pedrottimark commented 8 years ago

Distinguish general code in the introduction of no-shadow from incorrect code:

general-incorrect
pedrottimark commented 8 years ago

Distinguish incorrect code from correct code for the default option of comma-dangle:

incorrect-correct-1
pedrottimark commented 8 years ago

Distinguish incorrect code from correct code in adjacent options of no-shadow:

incorrect-correct-2
ilyavolodin commented 8 years ago

That looks nice, and might work in terms of markup. Although it means we need to go and put paragraphs with classes around each code example. I'm not sure you can do that with markdown.

pedrottimark commented 8 years ago

I was thinking the replace rules take effect after the conversion from markdown to HTML?

So that the only requirement in the content is consistency in the beginning of the sentence.

Imitating the following:

replace: '(recommended)', '<span title="recommended" aria-label="recommended" class="glyphicon glyphicon-ok"></span>'

replace: '(fixable)', '<span title="fixable" aria-label="fixable" class="glyphicon glyphicon-wrench"></span>'

ilyavolodin commented 8 years ago

You are right, replace kicks in after conversion. Sorry, didn't notice that you mentioned replace. That might work

ilyavolodin commented 8 years ago

I'll try to do this over the weekend and see if it works.

pedrottimark commented 8 years ago

Here is the CSS I used to make those picture:

p.incorrect + div > pre { background-color: hsl(0,100%,96%); }
p.correct + div > pre { background-color: hsl(120,100%,96%); }
pedrottimark commented 8 years ago

What do you think about this blue/yellow color contrast, which is more accessible than red/green?

no-negated-in-lhs_accessible

Here are the recommended changes:

ilyavolodin commented 8 years ago

While I understand that red/green is less accessible to some, it's much more standard to most. I think I would just stick with light red/green for incorrect/correct examples.

IanVS commented 8 years ago

I think red/green is okay, especially since we are not relying on the color coding. We've all made it this long without it, after all.