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

Admonition directives #47

Merged
merged 4 commits into from
Oct 28, 2016
Merged

Conversation

jonas
Copy link
Contributor

@jonas jonas commented Oct 26, 2016

screen shot 2016-10-27 at 10 36 57 pm

@jonas
Copy link
Contributor Author

jonas commented Oct 26, 2016

A WIP branch to use this in Akka HTTP: jonas/akka-http@9e8c0be

@ktoso
Copy link
Contributor

ktoso commented Oct 26, 2016

Awesome! :-)

@@ -366,3 +366,20 @@ case class VarsDirective(variables: Map[String, String]) extends ContainerBlockD
}
}
}

/**
* Adminition directive.
Copy link
Member

Choose a reason for hiding this comment

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

Admonition is misspelt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed

@jonas jonas force-pushed the admonition-directives branch from 444d125 to e599f79 Compare October 26, 2016 21:59
Copy link
Contributor

@eed3si9n eed3si9n left a comment

Choose a reason for hiding this comment

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

Some comments.
Also could you add a release note here please for 0.2.6? https://github.com/lightbend/paradox/tree/master/notes

@@ -0,0 +1,72 @@
Additional directives
Copy link
Contributor

Choose a reason for hiding this comment

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

We need a better name here, otherwise this page either will be kitchen sink of directives, or we'll have more-additional-directives.md. Suggestion is "Callout box" callout-box.md. Who knows what a callout is? Hey it's better than "Additional directives".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I didn't do a better job at researching a good name. Turns out that foundation has callouts.

@@ -1,7 +1,7 @@
Linking
-------

### @ref link
## @ref link
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't Linking with --- an h2? Why is @ref getting a promotion to h2 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, true. My main goal was to make the side menu more useful.

@@ -3,29 +3,48 @@
Templating
----------

Paradox uses [StringTemplate][st] for the basic templating. For example:
Paradox uses [StringTemplate][st] for the basic templating. Template files have
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this commit should be split into another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

*
* Replaces property values in verbatim blocks.
*/
case class AdmonitionDirective(name: String, defaultTitle: String) extends ContainerBlockDirective(Array(name): _*) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about renaming to CalloutDirective?

Copy link
Member

Choose a reason for hiding this comment

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

Blame the other Jonas, @jboner, for "admonition" :-)

akka/akka@b420866#diff-a4b6cde48a490dc1674bf1c440af6f22R198

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least blame Sphinx!

Makes `~docs/paradox` work as expected.
@dwijnand
Copy link
Member

I need to find a way to hold your PR hostage so you can ship me jonas/tig#313 :-P

@jonas jonas force-pushed the admonition-directives branch from e599f79 to d4e3ee1 Compare October 27, 2016 02:23
@jonas
Copy link
Contributor Author

jonas commented Oct 27, 2016

Let me know if I should leave foundation's background color for warnings.

screen shot 2016-10-26 at 10 26 27 pm

@jonas
Copy link
Contributor Author

jonas commented Oct 27, 2016

@dwijnand I got the rendering part working parsing standout control characters emitted by diff-highlight. I'll see if I can find some time to revive that patch. :-)

@eed3si9n
Copy link
Contributor

As per color palette, my policy around the "generic" theme was to make it feel neutral as possible, which means make it kind of look like Github's documentation. See their warning and note callouts here:

@sirthias
Copy link
Contributor

Awesome stuff! Really looking forward to this!

@pvlugter
Copy link
Member

Nice one!

@jonas jonas force-pushed the admonition-directives branch from d4e3ee1 to df2018b Compare October 28, 2016 02:33
@jonas
Copy link
Contributor Author

jonas commented Oct 28, 2016

@eed3si9n I personally find the warning colors a bit too dark which is why Foundation's warning color was left as is.

Some examples: http://codepen.io/anon/pen/RGmgRx
The updated branch uses example 3.
I can add the pure version of GitHub and Foundation's callout if you want.
screen shot 2016-10-27 at 10 20 12 pm
Note, I probably won't have time to play with colors before Monday, so let me know if I should split out the 3 first commits for a release of 0.2.6 without callouts.

@eed3si9n
Copy link
Contributor

eed3si9n commented Oct 28, 2016

3 looks good. Thanks for working on this.

@eed3si9n eed3si9n merged commit 790f972 into lightbend:master Oct 28, 2016
@jonas jonas deleted the admonition-directives branch October 28, 2016 02:57
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.

6 participants