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

More comprehensive docs for ol.DeviceOrientation #978

Merged
merged 3 commits into from
Sep 6, 2013

Conversation

pagameba
Copy link
Member

@pagameba pagameba commented Sep 5, 2013

Proposing an extended class description of DeviceOrientation along with
some additional details in the property accessors. Travis is running
its extensive tests, but it passes local linting and doc generation.

Adding an extended class description of DeviceOrientation along with
some additional details in the property accessors.
@@ -91,7 +138,8 @@ ol.DeviceOrientation.prototype.orientationChange_ = function(browserEvent) {


/**
* @return {number|undefined} alpha.
* @return {number|undefined} alpha The alpha value of the DeviceOrientation,
Copy link
Member

Choose a reason for hiding this comment

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

I think the parameter name alpha here is unnecessary. The return should just be the {type} followed by A complete sentence.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we just use {type} followed by {A complete sentence} everywhere else.

@tschaub
Copy link
Member

tschaub commented Sep 5, 2013

Awesome. Curious to see what the effect of the @class annotation is. Will check a build of the docs with/without.

@tschaub
Copy link
Member

tschaub commented Sep 5, 2013

I see that ol.Map and ol.View2D also use the @class annotation. The effect is that the containing element gets a class-description CSS class name instead of a vanilla description class name. In addition, the description shows up before the "name" (displayed as new DeviceOrientation) when you use the @class annotation. Without the annotation, the description shows up after the name.

With @class:
with-class

Without @class:
without-class

We're not consistent in the rest of the library, but my preference would be to avoid the extra annotation as it will likely be forgotten in other docs. But we could decide to get consistent and change this across the board in a separate issue.

@tschaub
Copy link
Member

tschaub commented Sep 5, 2013

This is a really great addition @pagameba. Thanks for the effort. If you get a chance, I do think the generated docs are nicer without the unnecessary parameter name in the return. Either way, it's is certainly worth merging.

@elemoine
Copy link
Member

elemoine commented Sep 5, 2013

We're not consistent in the rest of the library, but my preference would be to avoid the extra annotation as it will likely be forgotten in other docs. But we could decide to get consistent and change this across the board in a separate issue.

+1 from me for this change.

This removes @Class and fixes the return type pattern to match the
project standard.  There are a couple of minor changes to the text as
well.
@pagameba
Copy link
Member Author

pagameba commented Sep 5, 2013

I'm ambivalent about the use of @Class, if I had an opinion I would say its a bit nicer to have the class-level stuff before the constructor but I will follow whatever you guys decide is the best pattern. If you want to remove @Class, I can do a pull request to make it consistent through the rest of the docs.

Last commit removes @Class and fixes the pattern for the return types, sorry about that I was thinking of @param when I did it. I also tweaked the text in a couple of places.

@pagameba
Copy link
Member Author

pagameba commented Sep 5, 2013

Another commit, this one adds the inline link I wanted near the beginning of the class documentation. I'm still not sure I like this without @Class. Comments?

elemoine pushed a commit that referenced this pull request Sep 6, 2013
More comprehensive docs for ol.DeviceOrientation
@elemoine elemoine merged commit bebe391 into openlayers:master Sep 6, 2013
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.

3 participants