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

Switching tabs as well as other fragments #107

Merged
merged 16 commits into from
May 12, 2017

Conversation

raboof
Copy link
Contributor

@raboof raboof commented Apr 19, 2017

Expanding on the proposal in #95, added support for having other parts of
the documentation change as well when switching groups.

gsechaud and others added 5 commits February 1, 2017 15:15
Expanding on the proposal in lightbend#95, added support for having other parts of the
documentation change as well when changing a group.
@raboof
Copy link
Contributor Author

raboof commented Apr 19, 2017

Refs #106

@pvlugter
Copy link
Member

pvlugter commented Apr 19, 2017

The text switching on the spans looks good. Having a general purpose span directive is useful. My first thought for frequently used text groups is whether it would be useful to have it add extra named span directives, rather than just using span classes. Say specifying the possible groups in the build, since these are normally something like Java/Scala and used across the docs, and then having a named span directive like @scala[Scala only text]. So that it will warn you if use an unintended group or have a typo, and the syntax is more direct. Maybe using the group name directly doesn't work and it needs something else, like @for-scala or @scala-only or some other convention. Knowing what the possible groups are at the highest level, to add overall switches at the page level, could also be useful.

@raboof
Copy link
Contributor Author

raboof commented Apr 20, 2017

I like the idea of configuring 'supergroups' in the build (instead of the directive I added for that) and generating directives for them. Using the group name as directive name should be fine (you'll just have to make sure not to choose group names that clash with other directives) - otherwise @scala-group[Scala only snippet] might work.

This will also help when adding overall switches, too, indeed - and perhaps when implementing #14 we'll actually need it for generating a PDF for each possible selection.

Copy link
Contributor

@2m 2m left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple of nitpicks. Also it would be useful to have implicit group names assigned for @scaladoc and @JavaDoc directives (mentioned initially here)

@@ -311,7 +311,8 @@ case class SnipDirective(page: Page, variables: Map[String, String])
} else new File(page.file.getParentFile, source)
val text = Snippet(file, labels)
val lang = Option(node.attributes.value("type")).getOrElse(Snippet.language(file))
new VerbatimNode(text, lang).accept(visitor)
val group = Option(node.attributes.value("group")).getOrElse("")
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 the idea mentioned here: #95 (comment)

That would allow easier adaptation of the group functionality. All of the already written docs could use this feature without any changes to the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely agreed ;)

raboof added 3 commits April 20, 2017 16:46
Removes the need to define the 'supergroup' in the markdown.

Next up is of course switching from the dropdown.
First-scala
: @@snip [example-first.scala](../../resources/tab-switching/examples.scala) { #scala_first group=scala }

Some separator with text describing the @java[Java variant] @scala[Scala variant containing ***markdown*** and @ref:[Linking](linking.md)]
Copy link
Contributor

Choose a reason for hiding this comment

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

great 👍

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

LGTM, I like this a lot - can't wait to use it

@2m
Copy link
Contributor

2m commented Apr 24, 2017

When merging, make sure to create a merge commit (as opposed to squash and merge), as this PR contains commits from more than one author.

: @@snip [example-second.java](../../resources/tab-switching/examples.java)

Scala
: @@snip [example-second.scala](../../resources/tab-switching/examples.scala)
Copy link
Contributor

Choose a reason for hiding this comment

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

The example markdown does not match the markdown for render.

@2m
Copy link
Contributor

2m commented Apr 24, 2017

Its a pity I was not able to try it out, as paradox itself does not use the same code to generate the docs.

For snippets, the group now defaults to the type of snippet (e.g. java/scala).
@pvlugter
Copy link
Member

pvlugter commented May 4, 2017

I tried this out on the Cinnamon docs. The Cinnamon docs also uses tabs that shouldn't be switched. We have example Configuration snippets with a second tab called Defaults that displays the default configuration from a reference.conf (for example). It would be good if the switching could be disabled for these. It would also be great to support tabs without snippets (with text or direct code examples). I also got javascript errors for a direct code example in a tab.

Given that we now have a declarative list of the switchable groups in the build, I'm wondering if we could use that as a whitelist for switching (by injecting it into the template for the javascript part). And instead of looking for classes on code blocks in tabs, could we simply use the tab name itself (and match against the known groups). So for the Cinnamon docs, we would define the switchable tabs as "language" -> ("scala, "java), "build tool" -> ("sbt", "maven", "gradle"). And the configuration tabs would be left as is. We could also store something like language=java build-tool=gradle in the cookie then, rather than appending the latest group.

@raboof
Copy link
Contributor Author

raboof commented May 8, 2017

@pvlugter yes that makes a lot of sense to me!

@eed3si9n
Copy link
Contributor

What's the status on this PR? Is it ready to be merged?

@raboof
Copy link
Contributor Author

raboof commented May 10, 2017

@eed3si9n I think it might make sense to implement the ideas by @pvlugter before merging, and perhaps add hooks to customize the behavior on tab change (as e.g. for the akka docs we might want to do a page refresh instead of client-side switching until we've migrated all pages to support that). @pvlugter WDYT?

@eed3si9n
Copy link
Contributor

ok. Sounds good.

@pvlugter
Copy link
Member

SGTM

@pvlugter
Copy link
Member

@raboof are you going to work on these changes?

@raboof
Copy link
Contributor Author

raboof commented May 11, 2017

Yes if that doesn't interfere with your work I think I can look into it today!

@pvlugter
Copy link
Member

Great, sounds good.

raboof added 3 commits May 11, 2017 13:53
Though definition lists that don't contain any code snippets currently don't
become tabs, this makes things a bit more robust.
@raboof
Copy link
Contributor Author

raboof commented May 11, 2017

I've made some changes, including:

  • only switch groups declared in the paradox config
  • switch based on tab name when group property is missing (should be the common case)
  • use 'neater' cookie format
  • don't assume tabs contain snippets so heavily
  • allow extending group switch javascript
  • added documentation

I think this makes this PR ready for merge, review welcome.

Copy link
Member

@pvlugter pvlugter left a comment

Choose a reason for hiding this comment

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

Nice! LGTM!

Also tried it out on the Cinnamon docs and works great there.

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