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

added toggle raw #117

Merged
merged 2 commits into from Sep 26, 2018
Merged

added toggle raw #117

merged 2 commits into from Sep 26, 2018

Conversation

@Tavistock
Copy link
Contributor

Tavistock commented Sep 22, 2018

Sometimes the renderer mangles some quasi md in docstrings. I started to think about extending the md parser but i think it's an intractable problem. so instead add a option to just render the raw docstring. Motivating example in the images from re-frame.std-interceptors's docstring:

screen shot 2018-09-22 at 3 20 25 pm

screen shot 2018-09-22 at 3 20 58 pm

@martinklepsch

This comment has been minimized.

Copy link
Member

martinklepsch commented Sep 23, 2018

Thanks 🙏

In this particular case I would argue that the docstring is buggy — as in it is using markdown, but improperly in this one particular paragraph.

I think this particular issue is also somewhat common because Clojure docstrings tend to have weird whitespace at the beginning of lines due to indentation (see correct-indent in the codox code).

I wonder what could break if we would just prepend all lines indented by 2 spaces by another 2 spaces. Can’t think of a situation where users would consciously add 2 spaces to the beginning of a line... 🤔

Anyway, I think in either case this toggle raw switch is a nice idea, especially for longer docstrings of projects that are not using markdown.

{:escape-html? true
:render-wiki-link (comp render-wiki-link parse-wiki-link)})
hiccup/raw)])
hiccup/raw)]
[:pre.lh-copy.raw.hide (hiccup/raw doc-str)]])

This comment has been minimized.

Copy link
@martinklepsch

martinklepsch Sep 23, 2018

Member

hiccup/raw will prevent escaping of HTML in the passed string, I think in this case we do want any HTML in the doctoring to be escaped.

This comment has been minimized.

Copy link
@Tavistock

Tavistock Sep 23, 2018

Author Contributor

I removed the call which should escape html


.hide {
display: none;
}

This comment has been minimized.

Copy link
@martinklepsch

martinklepsch Sep 23, 2018

Member

cljdoc mostly uses Tachyons (see ADR #0011) for styling, basically using utility classes right in the markup. I think in this case we probably want something like the following:

  • .code(tachyons docs) for monospaced font, which is also whats used for arglists. Although an argument could be made that your stack is nicer since it uses SFMono when available.
  • .pa3 1rem padding
  • .br2 for 4px border-radius
  • .f6 for font-size
  • .overflow-x-scroll for, well... overflow-x: scroll

For the background color you can see if you find something here: https://tachyons.io/docs/themes/skins/

finally you can eliminate the .hide class by using the display helpers:

  • .dn display none
  • .db display block

This comment has been minimized.

Copy link
@Tavistock

Tavistock Sep 23, 2018

Author Contributor

I updated the styling to use tachyons

})
}
addToggleHandlers()
}

This comment has been minimized.

Copy link
@martinklepsch

martinklepsch Sep 23, 2018

Member

The file uses 2 spaces for indentation so far which might cause trouble the next time someone auto indents it. Do you think we should switch to 4 spaces or keep 2? I realize 2 is somewhat non-standard in JS 🙂

This comment has been minimized.

Copy link
@Tavistock

Tavistock Sep 23, 2018

Author Contributor

Spacemacs defaulted to 4. I have no js style opinions. I suggest we go with the original style of 2.

@@ -100,7 +102,8 @@
[:a.link.f7.gray.hover-dark-gray.mr2
{:href (platf/get-field def :src-uri p)}
(format "source (%s)" p)])
[:a.link.f7.gray.hover-dark-gray {:href (platf/get-field def :src-uri)} "source"]))]))
[:a.link.f7.gray.hover-dark-gray.mr2 {:href (platf/get-field def :src-uri)} "source"]))
[:a.link.f7.gray.hover-dark-gray.toggle-raw {:href "#"} "toggle raw"]]))

This comment has been minimized.

Copy link
@martinklepsch

martinklepsch Sep 23, 2018

Member

It would be nice if the button would alternate between raw docstring / formatted docstring depending on the state of the UI.

This comment has been minimized.

Copy link
@martinklepsch

martinklepsch Sep 23, 2018

Member

In some other places there has been a loose convention of prepending JS-relevant classnames with js-- so I guess we could also make this one js--toggle-raw?

This comment has been minimized.

Copy link
@Tavistock

Tavistock Sep 23, 2018

Author Contributor

done and done

@martinklepsch

This comment has been minimized.

Copy link
Member

martinklepsch commented Sep 23, 2018

I want to make merging of PRs as easy as possible for contributors while still providing feedback that will allow people to contribute more effectively in the future if they choose to.

With that in mind we can

  1. Merge this PR as is and I'll make the few adjustments I requested in the comments
  2. Make changes while this PR is open until it's 'ready'

Let me know what you prefer, I'm happy about this contribution either way 🤗

@Tavistock

This comment has been minimized.

Copy link
Contributor Author

Tavistock commented Sep 23, 2018

option 2 is fine. I'll address all the issues in the comments and push up some new commits.

@Tavistock

This comment has been minimized.

Copy link
Contributor Author

Tavistock commented Sep 23, 2018

the are also ns doctrings that i didnt even think about. I'll look into adding a raw button for them too

@martinklepsch martinklepsch merged commit 4887888 into cljdoc:master Sep 26, 2018
@martinklepsch

This comment has been minimized.

Copy link
Member

martinklepsch commented Sep 26, 2018

@Tavistock merged this now, feel free to open more PRs for the namespace docstring 👍 thanks!

@martinklepsch

This comment has been minimized.

Copy link
Member

martinklepsch commented Oct 1, 2018

I just deployed this, thanks again very much Travis! Here's a quick demo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.