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

Docs: Visible type information. #13308

Merged
merged 5 commits into from
Feb 16, 2018
Merged

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Feb 13, 2018

This came up in an issue at one point, so thought I'd suggest a way of displaying type information in the docs. This just covers BufferGeometry, but I can update other pages if the approach is agreeable. PR now covers all pages. Method/property names would now use their own permalinks, with type information in a low-contrast color afterward.

Demo — All docs

Screenshot
screen shot 2018-02-12 at 7 43 37 pm

@mrdoob
Copy link
Owner

mrdoob commented Feb 13, 2018

That looks great to me! 👍👍👍

@mrdoob mrdoob added this to the r91 milestone Feb 13, 2018
@WestLangley
Copy link
Collaborator

I do not think the foreground/background contrast, as proposed, is sufficiently legible.

There are metrics for these things, but I expect someone could select reasonably-high-contrast colors and they would be fine.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Feb 13, 2018

Pushed an update with higher contrast, matching the comments in our code blocks. I'll wait to update other pages until after r90 to avoid drift/conflicts on the PR and to get any other feedback.

@WestLangley
Copy link
Collaborator

Pushed an update with higher contrast

I think a different color scheme for the entire document is in order, quite frankly. I vote for clearly legible.

@donmccurdy
Copy link
Collaborator Author

Let's address that in another PR — showing type information at all, with the same legibility as example comments (slightly more legible, actually, since the background is lighter), is an improvement on hover-text-only, especially for touchscreen users. 🙂

@looeee
Copy link
Collaborator

looeee commented Feb 13, 2018

This is awesome! 😄

@donmccurdy donmccurdy force-pushed the docs-visible-types branch 2 times, most recently from f04c59a to ce4fd0b Compare February 16, 2018 01:57
@donmccurdy
Copy link
Collaborator Author

A few prayers to the regex gods later, this PR should cover all of the docs now. 😅

@takahirox
Copy link
Collaborator

takahirox commented Feb 16, 2018

I'm sure this change is very helpful to users.

One thing, "return value type null" for the methods returning nothing would be a bit misleading. So should be like this?

.dispose () : nothing
.dispose () : undefined
.dispose () :

This seems existing issue, so another PR would be good.

@donmccurdy
Copy link
Collaborator Author

Good point. Hiding the return type when there is no return value seems cleanest. Updated.

@Mugen87
Copy link
Collaborator

Mugen87 commented Feb 16, 2018

One minor thing. It seems that syntax highlighting is broken:
image

It should look like this:

image

docs/page.js Outdated
text = text.replace( /\[(?:member|property|method):([\w]+) ([\w\.\s]+)\]/gi, "<a onclick=\"window.parent.setUrlFragment('" + name + ".$2')\" target=\"_parent\" title=\"" + name + ".$2\" class=\"permalink\">#</a> .<a onclick=\"window.parent.setUrlFragment('$1')\" title=\"$1\" id=\"$2\">$2</a> " );
text = text.replace( /\[(member|property|method|param):([\w]+)\]/gi, "[$1:$2 $2]" ); // [member:name] to [member:name title]
text = text.replace( /\[(?:member|property|method):([\w]+) ([\w\.\s]+)\]\s*(\(.*\))?/gi, "<a onclick=\"window.parent.setUrlFragment('" + name + ".$2')\" target=\"_parent\" title=\"" + name + ".$2\" class=\"permalink\">#</a> .<a onclick=\"window.parent.setUrlFragment('" + name + ".$2')\" id=\"$2\">$2</a> $3 : <a class=\"param\" onclick=\"window.parent.setUrlFragment('$1')\">$1</a>" );
text = text.replace( / : <a class="param" onclick="window.parent.setUrlFragment\('\w+'\)">null<\/a>/gi, '' ); // remove null return types
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we need this line. What's wrong with showing : null?

Copy link
Owner

Choose a reason for hiding this comment

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

Lets remove this line and the PR will be good to go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Feb 16, 2018

One minor thing. It seems that syntax highlighting is broken:

Strange... I think that's caused by Rawgit — there are errors on the console about MIME types of the code highlighting CSS. It works when serving the dev branch, but perhaps can't do the same from secondary branches? E.g. this older branch shows the same issue.


@mrdoob I think you left a comment about handling null returns more simply, but I can't find it inline, maybe GitHub is struggling with the large diff... Just guessing here, we could change all [member:name title] entries inline to something more like:

[member:title] () : [return:name]

... where the : [return:name] portion is omitted for methods returning null. I think that would result in 1 fewer lines of code, because the [member:name] to [member:name title] regex could be removed. Preference?

EDIT: Oh there's your comment 😅 yeah i'm fine with leaving : null in, too. Technically we are not returning null, but it's clear enough that I don't have a preference really.

@mrdoob
Copy link
Owner

mrdoob commented Feb 16, 2018

When a method return null tends to have a reason. If anything... I would hide the return part when a method returns undefined as that is the default.

@donmccurdy
Copy link
Collaborator Author

Codebase has 357 matches for [method:null and none for [methed:undefined. I think we've used null in all of the cases where nothing is returned, too, so I can't differentiate here without going through method by method. 😕

@takahirox
Copy link
Collaborator

takahirox commented Feb 16, 2018

Personally I wanna be strict with value type representation as much as possible so I wanna distinguish between return null and return nothing.

But, as I said, we can leave : null so far and fix that in another PR (if necessary). The main purpose of this PR is making value type visible in doc, not updating return value type. And updating would take time because we can't automatically differentiate them, perhaps we need to update by hand.

@mrdoob
Copy link
Owner

mrdoob commented Feb 16, 2018

But, as I said, we can leaving : null so far and fix that in another PR (if necessary).

Yes please.

@donmccurdy
Copy link
Collaborator Author

Sounds good, removed the line. Glad to consider other changes in another PR. 👍

@mrdoob mrdoob merged commit 29cd698 into mrdoob:dev Feb 16, 2018
@mrdoob
Copy link
Owner

mrdoob commented Feb 16, 2018

Thanks!

@donmccurdy donmccurdy deleted the docs-visible-types branch March 7, 2018 15:18
@mrdoob
Copy link
Owner

mrdoob commented Mar 8, 2018

After this PR, I guess the # link in the right side of every property/method is no longer needed?

@donmccurdy
Copy link
Collaborator Author

Not functionally necessary, no. I like the pattern where the # appears to the left when you hover over a title, just as a clue it's a permalink. But either way.

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