-
Notifications
You must be signed in to change notification settings - Fork 77
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
Change links to snippet source from text to icon #260
Conversation
@@ -28,7 +28,7 @@ import scala.collection.JavaConverters._ | |||
class AnchorLinkSerializer extends ToHtmlSerializerPlugin { | |||
def visit(node: Node, visitor: Visitor, printer: Printer): Boolean = node match { | |||
case anchor: AnchorLinkSuperNode => | |||
printer.print(s"""<a href="#${anchor.name}" name="${anchor.name}" class="anchor"><span class="anchor-link"></span></a>""") | |||
printer.print(s"""<a href="#${anchor.name}" name="${anchor.name}" class="anchor"><span class="anchor-link icon"></span></a>""") |
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.
Is this necessary? It doesn't look related to snippets and the anchor-link
class is already pretty specific.
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.
I extracted the .icon
CSS class to be general so it could be reused here and on the snippet source links.
Should I have done it differently?
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.
The properties can also be shared by listing the selectors:
a.anchor .anchor-link, .icon {
/* ... */
}
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.
Fair enough. Pushed a commit that reverts the changes to the scripted tests and now shares properties across selectors.
node match { | ||
case vgn: VerbatimGroupNode => | ||
vgn.getSourceUrl.ifPresent(sourceUrl => { | ||
printer.print(s"""<a class="icon go-to-source" href="$sourceUrl" target="_blank" title="Go to snippet source"></a>""") |
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.
Ah, I like the "open in new tab".
if (variables.contains(GitHubResolver.baseUrl) && | ||
variables.getOrElse(SnipDirective.showGithubLinks, "false") == "true") { | ||
val p = resolvePath(page, Path.toUnixStyleRootPath(file.getAbsolutePath), labels.headOption).base.normalize.toString | ||
new ClassyLinkNode(p, "snippet-full-source github", new TextNode("Full source at GitHub")).accept(visitor) |
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.
Shouldn't the snippet-full-source
class be removed again with this PR?
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.
I did not find a mention of snippet-full-source
anywhere else in Paradox. So it seems it was not even used for styling or anything else.
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.
I added it so that I could style the element in the paradox material theme. 👍 for adding something equivalent in the new version.
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.
Currently the new links have class="icon go-to-source"
. Is that OK?
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.
Yes, that's perfect. Thanks a lot.
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.
LGTM
From discussion in https://github.com/akka/akka-paradox/issues/36