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

feat(GridLayout): Add synonym property column for col in GridLayout by mehulcs · Pull Request #7368 · NativeScript/NativeScript · GitHub

feat(GridLayout): Add synonym property column for col in GridLayout - #7368

Closed
mehulcs wants to merge 5 commits into
NativeScript:masterfrom
mehulcs:grid-7295
Closed

mehulcs wants to merge 5 commits into
NativeScript:masterfrom
mehulcs:grid-7295

Conversation

mehulcs commented Jun 18, 2019
edited
Loading

Copy link
Copy Markdown

What is the current behavior?

There is no column property synonym of col.

What is the new behavior?

Added new property column synonym for col.

Fixes/Implements/Closes #7295 .

cla-bot Bot commented Jun 18, 2019

Copy link
Copy Markdown

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign the CLA at https://www.nativescript.org/cla.
CLA has not been signed by users: @mehulcs.
After signing the CLA, you can ask me to recheck this PR by posting @cla-bot check as a comment to the PR.

mehulcs commented Jun 18, 2019

Copy link
Copy Markdown
Author

@cla-bot check

cla-bot Bot added the cla: yes label Jun 18, 2019

cla-bot Bot commented Jun 18, 2019

Copy link
Copy Markdown

The cla-bot has been summoned, and re-checked this pull request!

vakrilov added the docs needed Additional documentation on this issue/PR is needed label Aug 1, 2019

Copy link
Copy Markdown
Contributor

test

vakrilov commented Aug 2, 2019

Copy link
Copy Markdown
Contributor

test

vakrilov commented Aug 5, 2019
edited
Loading

Copy link
Copy Markdown
Contributor

Hi @mehulcs
First of all - thanks for the PR!

There are some failing unit tests in your PR currently. They are caused by the logic in the getColumn and getColumnSpan which select the value from one of the "synonym" over the other.

Giving it a further thought - as long as there are 2 properties for one value (ex. col and column) there will always be a problem so syncing these properties with one another or having to choose which one is to use.

I have another more simple and easy to maintain approach. Define the column and columnSpan properties as javascript properties with getters and setters that will just fallback to the original property:

Object.defineProperty(View.prototype, "column", {
    get(this: View): number { return this.col; },
    set(this: View, value: number) { this.col = value; },
    enumerable: true,
    configurable: true
});

Object.defineProperty(View.prototype, "columnSpan", {
    get(this: View): number { return this.colSpan; },
    set(this: View, value: number) { this.colSpan = value; },
    enumerable: true,
    configurable: true
});

This way we won't have to deal with syncing issues.

I've tested this approach in this branch(based on your initial PR) - it seems to work as expected.

mehulcs commented Aug 5, 2019

Copy link
Copy Markdown
Author

Thanks, @vakrilov @manoldonev
@vakrilov That's a wonderful approach to make synonym property, Let me know if there are any changes required to be made in this PR.

vakrilov commented Aug 5, 2019

Copy link
Copy Markdown
Contributor

Hey @mehulcs - I have extended your PR and re-opened it in #7641.
Your initial commits are still there, so you will be recognized as contributor.

Thanks!

vakrilov closed this Aug 5, 2019

mehulcs commented Aug 5, 2019

Copy link
Copy Markdown
Author

Thanks @vakrilov

vakrilov removed the docs needed Additional documentation on this issue/PR is needed label Aug 5, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make column a synonym for col property re layout child properties

3 participants


Back | FazBrowse Home | New Git URL