FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(core/properties): Fix typings for nativeValueChange by the-reality-engineer · Pull Request #5791 · NativeScript/NativeScript · GitHub

/ NativeScript Public

fix(core/properties): Fix typings for nativeValueChange - #5791

Merged
vakrilov merged 7 commits into
NativeScript:masterfrom
the-reality-engineer:patch-1
Jun 19, 2018
ConversationCommits (7)ChecksFiles changed
Merged

fix(core/properties): Fix typings for nativeValueChange#5791
vakrilov merged 7 commits into
NativeScript:masterfrom
the-reality-engineer:patch-1

Conversation

the-reality-engineer commented May 6, 2018

Copy link
Copy Markdown
Contributor

No description provided.

ghost added the ♥ community PR label May 6, 2018
ns-bot added the cla: yes label May 6, 2018
manoldonev reviewed May 8, 2018
View reviewed changes
Comment thread tns-core-modules/ui/core/properties/properties.d.ts Outdated
public readonly defaultValue: U;
public register(cls: { prototype: T }): void;
public nativeValueChange(T, U): void;
public nativeValueChange(target:T, value:U): void;

manoldonev May 8, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

First argument is generally referred as owner across the codebase:

NativeScript/tns-core-modules/ui/core/properties/properties.ts

Line 70 in 7edf561

public readonly nativeValueChange: (owner: T, value: U) => void;

Can you update this?

ghost assigned manoldonev May 8, 2018
ghost added in progress and removed ♥ community PR labels May 8, 2018
manoldonev added this to the 4.1 milestone May 21, 2018
manoldonev removed this from the 4.1 milestone May 21, 2018
dtopuzov modified the milestone: 4.1 May 21, 2018

vakrilov commented Jun 19, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hey @speigg
I think this PR is almost ready to be merged. Do you mind implementing @manoldonev 's suggestion about the owner param and add this to master? Also be sure to run npm run tslint and fix the linter errors.
Thanks!

Update properties.d.ts
3edbc7b 
Changed parameter name of nativeValueChange from “target” to “owner”
Fixed white space to match coding style
Update properties.d.ts
4a87ece 
Fixed white space

the-reality-engineer commented Jun 19, 2018

Copy link
Copy Markdown
Contributor Author

I made the changes directly on Github.com, as I don’t have the {N} repository downloded and setup for development. Anyway, the CI seems to be happy about the whitespace now.

ghost assigned vakrilov Jun 19, 2018

vakrilov commented Jun 19, 2018

Copy link
Copy Markdown
Contributor

test

vakrilov merged commit 357c8ec into NativeScript:master Jun 19, 2018
ghost removed the in progress label Jun 19, 2018

lock Bot commented Aug 26, 2019

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

lock Bot locked and limited conversation to collaborators Aug 26, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
manoldonev manoldonev left review comments
vakrilov vakrilov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

manoldonev

vakrilov

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL