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

Creation of liquid-based notes/documentation of approach #3253

Closed
wants to merge 1 commit into from

Conversation

johndmulhausen
Copy link

@johndmulhausen johndmulhausen commented May 12, 2017

See: https://deploy-preview-3253--docker-docs.netlify.com/test/#admonitions-notes

To use:

Note

{% include note.html type="note" title="Note example" text="Multiline txt is OK!

* See?
* It's fine!" %}

Important

{% include note.html type="important" title="Important example" text="How important?

* Very.
* Like, a lot." %}

Warning

{% include note.html type="warning" title="Warning example" text="Beware of dog

* He is ruff.
* Has a bone to pick." %}

img

@mdlinville
Copy link

I wish it wasn't so much to type. @allejo WDYT?

@joaofnfernandes
Copy link
Contributor

joaofnfernandes commented May 12, 2017

I really dislike this approach because two reasons:

  • We're essentially replicating a markdown feature but with a worse syntax
  • It encourages you to use any markdown syntax inside a note. And if you're promoting bad behavior, users are going to leverage that to do all kinds of nasty things.

An example of what I'm saying

> This is a markdown note
> * There's nothing Keeping me from doing
> * This, but **it feels** awkward, so you don't even
> * Know if _this is supported_ or *not*

Compared with

{% include note.html type="important" title="This is basically" text="HTML
So what's the point of this, when I can just write HTML in here
* and it feels
* comfortable
| Doing | All | Kinds | of nasty | things |
|:------|:---|:-------|:--------|:-------|
| yup   | it    | is         | almost  | html     |

and I could go on and on and write a big, beautiful,
amazing note, on my editor, that's going to look
like 💩 to end-users
" %}

@johndmulhausen
Copy link
Author

johndmulhausen commented May 12, 2017

I must confess I'm having trouble understanding why supporting full markdown unawkwardly inside a note is a bad thing.

Consider the alternative: Using a shifting list of CSS styles to pull off the styling/iconography we want. It's already so inconsistent. Adding glyphicons without a standardized approach is not going to work; I already deleted a massive wall of text with conflicting approaches in test.md to get here.

@allejo
Copy link
Contributor

allejo commented May 12, 2017

I think the {% include %}, even though a lot to type, would be easier to understand but you could also go with a pure markdown solution as such:

{:.note.warning}
> **Title**
>
> Beware of dog
>
> * He is ruff.
> * Has a bone to pick.

On a side note, the <em class="{{ glyphClass }}"></em> in the notes.html could be simplified to just using a CSS pseudo element (i.e. .some-element::before) and use CSS like so:

.important-vanilla p:first-of-type::before {
    content: '\f071';
    display: inline-block;
    font-family: FontAwesome; /* or whatever */
}

@londoncalling
Copy link
Contributor

Pros of this new approach as I see it are:

  1. consolidates admonitions formats and calls to associated icons for each note type in one place
  2. enables updating formats, icons, etc. to all admonitions by updating that single file

Cons:

  1. Joao's points above ^^

Reminders:

  1. Goals of 1st round of note format updates (_scss/notes.css) - add color coding per severity (3 levels), and, less critical, pre-pend a word to indicate severity (Note, Important, Warning) . All done in one file in CSS, great.
  2. Goal of 2nd round of updates (added more classes to _scss/notes.css) - provide a way to have notes without the pre-pended word so you can write totally customized admonition titles, and use pre-pended icons instead. This was all done by adding a few more classes to the same CSS file, and some inline HTML (embedded in the notes themselves) to call the icons.
  3. Goal of 3rd round (added _includes/content/admonitions/notes.html) - Get the pre-pended icons in there without having to paste them into the notes.

@joaofnfernandes
Copy link
Contributor

joaofnfernandes commented May 12, 2017

@allejo that markdown version is what I'd like to see, and we can even make things simpler by using good defaults.

Here's what I mean

> This should be a one-liner vanilla note. 

As a writer I don't need to add anything else, but by default this is a "note", so we can render this in yellow with whatever glyphicon makes sense.

> This is a warning
{.warning}

As a writer I'm changing the meaning of this note to be a warning. That means the color will change as well as the glyphicon.

Here's where things get more interesting.

> This is the title of my note
>
> And this is the body. Technically I can use whatever markdown I want
> But we should not be using it

As a writer this would apply the "note" color and glyphicon, but also make that title stronger (bigger, bolder, or something else)

> This is the title of my warning
>
> I can change the meaning of this warning
> By adding a class to the end of it
{.warning}

Again, this would do whatever a note with title does, but apply a different color and glyphicon.


How does this work? If you feed this to Kramdown

>
>

It will give you

<blockquote>
  <p></p>
</blockquote>

But if you give it

>

>

It will give you

<blockquote>
  <p></p>
  <p></p>
</blockquote>

So we can leverage that in the CSS, and assume that if you're using multiple paragraphs is because you want a title. That's what #2795 already did for you.

@johndmulhausen

I already deleted a massive wall of text with conflicting approaches in test.md to get here

That's because test.md had samples for the old way of doing notes, just to ensure #2795 didn't totally messed up nodes written like this

> **Note**: This is a note using
> the old style and has multiple lines, but a single paragraph

Which were almost all of them.
The ground work is already done for you, you just need to update _scss/_notes.scss replacing

content: 'Warning: '

By

content: '\f071';
font-family: FontAwesome; /* or whatever */

And you'll have glyficons while retaining a clean markdown syntax.

@johndmulhausen
Copy link
Author

johndmulhausen commented May 13, 2017

A few points:

  1. There should not be any one-liner notes. The fact that we can loosely format notes is a huge problem that needs to be solved.
  2. We shouldn't pre-pend text to notes. We have agreed as a team to use icons instead.
  3. We should not have notes that have no heading. They should be meaningful asides. Otherwise, blockquoting becomes enhancement confetti.
  4. Whatever we decide, the old ways need to be deleted from both test.md and the docs generally. I count a total of five documented ways of doing notes in test.md right now, and lord knows how many ways we use notes in the docs. There should be one, so that we present things consistently and so that all of our efforts to improve the authoring and user experiences for notes have maximum benefit for ourselves and our readers.
  5. Near as I can tell the actual use of glyphicons involves a combination of HTML + CSS class references. Specifically: <i class="glyphicon glyphicon-search"></i>. I don't know that there's a pure CSS or Markdown solution for this. Is this why we're discussing FontAwesome at this point? ::before with content: + formatting rules can not import an entire CSS class.
  6. If it's possible to have titled, consistent, icon-having notes without using Liquid, I am all ears. Without Liquid actually enforcing some expectations around these admonitions, I fear people will type whatever they like, and generally not bother with the somewhat precious CSS rules that we're talking about, and inconsistency will continue. Inconsistency is far and away the bigger problem. Getting icons is just icing.

@allejo
Copy link
Contributor

allejo commented May 13, 2017

Near as I can tell the actual use of glyphicons involves a combination of HTML + CSS class references. Specifically: . I don't know that there's a pure CSS or Markdown solution for this. Is this why we're discussing FontAwesome at this point?

@johndmulhausen oh no no. I only used FontAwesome as an example because I didn't know what icon font was being used. Similar to how you can prepend text to HTML elements in CSS with ::before and ::after, you can do the same with icons.

See here for an example of what I mean: https://jsfiddle.net/j9g8Lfgq/

@mdlinville
Copy link

Yeah that last thing is what I meant by doing it the same way the bootstrap CSS itself does it.

The one thing is that if we did it that way we need to make sure that the note admonition is in its own column so that the note text doesn't wrap underneath the admonition (like how it is currently in the fiddle).

@johndmulhausen
Copy link
Author

johndmulhausen commented May 13, 2017

Okay, with pure Markdown/CSS, can we somehow stop someone cold if they try to submit a note that looks like this?

> **Note**: I have a thing to say and I want blue indenting so that people think it's important

We want notes like this:

> **Be sure to use aufs volumes**
>
> Using btrfs volumes won't work in this situation. 
{: .important }

But there will be zero guarantee that this is what we will get if we don't tread carefully here. It throws a literal build error if you incorrectly format a note if we use Liquid.

And let's say we do want to do global improvements that change the HTML we use -- because we aren't in control of the HTML of the notes, it will be difficult to implement, when it would take minutes to update note.html.

@mdlinville
Copy link

@allejo
Copy link
Contributor

allejo commented May 13, 2017

Or if you want the title and content to line up, have the icon be position absolute and just add padding: https://jsfiddle.net/xzpku6bw/2/

@johndmulhausen
Copy link
Author

johndmulhausen commented May 13, 2017

^ While this is cool, I don't hear any answers re: consistency.

I'll add another point which is that I am completely fine with the outcome of there being less asides if we go this route. I'd rather have a small number of high-quality asides -- that's a win-win.

@joaofnfernandes
Copy link
Contributor

@johndmulhausen we can enforce consistency by reviewing the PRs and ensuring they conform to the styleguide. That's orthogonal to use markdown blockquotes or liquid templating.

From a technology standpoint there's nothing stopping me from using the template in this PR without a type or title. Liquid will happily compile that note and render the template.
Just the same, there's nothing stoping me from submitting a PR that uses blockquotes in a way that doesn't conform with our styleguide.

That's the reason we use PRs and don't merge unless the submission conforms with the styleguide.

@mdlinville
Copy link

Honestly if we want that level of consistency, I think we should be using DITA.

Copy link
Contributor

@londoncalling londoncalling left a comment

Choose a reason for hiding this comment

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

For a Liquid solution, this definitely works, but I have a slight preference for a solution that calls icons from the CSS (#3254). If we want to keep both approaches available and explore it more, I don't see much of a downside with keeping the new includes file and examples available, alongside the FontAwesome solution until we fully settle on one or the other. It would be nice if the naming of the two associated files matched (e.g., either notes.html and _notes.scss or note.html and _note.scss), currently they don't match.

@johndmulhausen
Copy link
Author

Well I'm not going to shove a particular approach down people's throats. The main thing I want is consistency; the means don't matter to me as much. Doesn't look like this is the route we want to take as a team.

Let's try to solve this with a stronger style guide and coalescing around the glyphicons+CSS approach @londoncalling implemented in #3254.

It's very important that we start taking consistency and accessibility more seriously. We already assigned a very large-scoped hackathon bug to this issue and awarded 500 points to it, but we still have an inconsistently applied approach that ignores accessibility for color blindness. If we can all agree that titled, iconed admonitions are best for users and make a true pattern out of it, I'll be stoked.

Thanks for the discussion @allejo @mstanleyjones @joaofnfernandes @londoncalling!

@johndmulhausen johndmulhausen deleted the liquidnotes branch May 15, 2017 18:53
@londoncalling
Copy link
Contributor

londoncalling commented May 15, 2017

@johndmulhausen the other solution, or any solution, will definitely need more refining.

  • We haven't really nicely addressed cases for admonitions that naturally are one-liners, and don't rise to the need for a bold title or any title. I'm hitting some of these cases in the Compose file reference and, for now, making as few changes as possible.

  • Also, there are some situations where it might be useful to have a pre-pended word. Currently, we do have the ability to choose either a pre-pended word format or icon format from the CSS formats.

But all this can be an interesting discussion for later in the week or so.

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.

5 participants