Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

t/41: Improved documentation of the Observable#bind method #253

Merged
merged 13 commits into from
Oct 26, 2018
Merged

Conversation

oleq
Copy link
Member

@oleq oleq commented Oct 2, 2018

Suggested merge commit message (convention)

Docs: Improved documentation of the Observable#bind method. Closes ckeditor/ckeditor5#4887.

@oleq oleq requested review from Reinmar and pjasiun October 2, 2018 15:03
@coveralls
Copy link

coveralls commented Oct 2, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 10d4cac on t/41 into ffc91df on master.

@Reinmar
Copy link
Member

Reinmar commented Oct 4, 2018

Could you move this to the deep dive section? I started moving long API docs like this out of API docs where there are hard to find and hard to read. You'll need to extend it a bit and link to it from some places in https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/framework/guides/architecture/ui-library.html and from API docs.

Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

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

Let's move it to the deep dive section of the documentation.

@oleq
Copy link
Member Author

oleq commented Oct 5, 2018

Move like "leave what I did here + extend it in the guide" or "discard what I did here and just write in the guide"?

@Reinmar
Copy link
Member

Reinmar commented Oct 5, 2018

Like: cut what you wrote, paste it into the new guide, review if it makes sense outside (it will miss a context, so most likely it needs to be extended with some intro), add cross-links between various places.

@oleq
Copy link
Member Author

oleq commented Oct 8, 2018

Ready for another round of review:

Having mixed the {@link module:utils/observablemixin~ObservableMixin} into your class, you can define observable properties. To do that, use the {@link module:utils/observablemixin~ObservableMixin#set `set()` method}. Let's set a couple of properties and see what they look like in a simple `Command` class:

```js
export default class Command {
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing. Especially the new Command( 'thing' ) used later on. We should base the docs on some realistic code and we don't write a code like this.

src/observablemixin.js Outdated Show resolved Hide resolved
src/observablemixin.js Outdated Show resolved Hide resolved
Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

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

See comments.

@Reinmar Reinmar removed the request for review from pjasiun October 26, 2018 12:19
@Reinmar
Copy link
Member

Reinmar commented Oct 26, 2018

I'll merge this guide now because I'm basing some other changes in another repo on them. You can commit some fixes for it straight to master, @oleq.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants