-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Conversation
I wish it wasn't so much to type. @allejo WDYT? |
I really dislike this approach because two reasons:
An example of what I'm saying
Compared with
|
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. |
I think the
On a side note, the .important-vanilla p:first-of-type::before {
content: '\f071';
display: inline-block;
font-family: FontAwesome; /* or whatever */
} |
Pros of this new approach as I see it are:
Cons:
Reminders:
|
@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
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.
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.
As a writer this would apply the "note" color and glyphicon, but also make that title stronger (bigger, bolder, or something else)
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
But if you give it
It will give you
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.
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
Which were almost all of them.
By
And you'll have glyficons while retaining a clean markdown syntax. |
A few points:
|
@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 See here for an example of what I mean: https://jsfiddle.net/j9g8Lfgq/ |
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). |
Okay, with pure Markdown/CSS, can we somehow stop someone cold if they try to submit a note that looks like this?
We want notes like this:
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. |
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/ |
^ 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. |
@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 That's the reason we use PRs and don't merge unless the submission conforms with the styleguide. |
Honestly if we want that level of consistency, I think we should be using DITA. |
There was a problem hiding this 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.
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 the other solution, or any solution, will definitely need more refining.
But all this can be an interesting discussion for later in the week or so. |
See: https://deploy-preview-3253--docker-docs.netlify.com/test/#admonitions-notes
To use:
Note
Important
Warning