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.

Sign up for GitHub

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

Jump to bottom

FLUID-6158: Document fluid.modelPairToChanges() #122

Merged
merged 4 commits into from
Nov 7, 2017
Merged

FLUID-6158: Document fluid.modelPairToChanges() #122

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
088e54a
FLUID-6158: Document fluid.modelPairToChanges()
simonbates Jun 12, 2017
09597e8
Merge remote-tracking branch 'upstream/master' into FLUID-6158
simonbates Aug 9, 2017
6c67d89
FLUID-6158: Use "changeRequest" rather than "change record"
simonbates Aug 9, 2017
211eed5
FLUID-6158: Document fluid.fireChanges
simonbates Aug 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/documents/CoreAPI.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,22 @@ if you are interested.
path segments representing this
* `newValue: {Any}` The value to be written into `model` at the path `path`

### fluid.modelPairToChanges(value, oldValue, changePathPrefix)

* `value {Any}` Model value to compare
* `oldValue {Any}` Model value to compare
* `changePathPrefix {String|Array of String}` [optional] Path prefix to prepend to change record paths
* Returns: `{Array of Object}` An array of change records
Copy link
Member

Choose a reason for hiding this comment

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

This type should really read {Array of changeRecord} and link to the section in ChangeApplierAPI which defines it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I was incorrect in referring to the output as "change records". Looking at the ChangeApplierAPI docs, I think they are changeRequest objects, because I output "path" properties rather than "changePath" properties. Does this seem right?

I've updated the docs to read "changeRequest" and added a link to the docs for it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is brilliant, thanks!


Calculates the changes between the model values `value` and `oldValue`
and returns an array of change records. The optional argument
`changePathPrefix` is prepended to the change path of each record (this
is useful for generating change records to be applied at a non-root path
in a model).
Copy link
Member

Choose a reason for hiding this comment

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

It would be really useful to have a super-simple example here that might allow people to get a sense of what is going on - for example, a computation of the changes between {a: 1} and {b:2}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion. Done.

Copy link
Member

Choose a reason for hiding this comment

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

Great example, very clear


The returned array of change records may be used with
`fluid.fireChanges()`.
Copy link
Member

Choose a reason for hiding this comment

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

This API isn't actually documented - I wonder if I could prevail on you to add some basic docs for it in ChangeApplierAPI.md at the end of the section "Programmatic style for triggering a change" and then link to it here - cheers!

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 this comment got displaced through collateral changes - do you think it seems doable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, absolutely. I will add docs for fluid.fireChanges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


### fluid.model.parseEL(EL)

* `EL {String}` The EL path to be parsed
Expand Down