Skip to content

Fix #1792(options): use gridOptions rather than copy them #1869

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

Merged
merged 1 commit into from
Oct 23, 2014

Conversation

PaulL1
Copy link
Contributor

@PaulL1 PaulL1 commented Oct 18, 2014

The current arrangement is to shallow copy gridOptions, which
means changes to scalar values are not automatically noticed.

Change to reference gridOptions directly, defaulting any necessary
values over the top. This then allows options to be referenced
and updated dynamically.

The current arrangement is to shallow copy gridOptions, which
means changes to scalar values are not automatically noticed.

Change to reference gridOptions directly, defaulting any necessary
values over the top.  This then allows options to be referenced
and updated dynamically.
@PaulL1
Copy link
Contributor Author

PaulL1 commented Oct 18, 2014

Not merging this, requires review by @c0bra and/or @swalters. Not clear to me whether a deliberate decision was taken not to reference options and to copy them instead, this could be a substantial change with implications I don't understand. All unit and e2e tests pass, but that doesn't mean something obscure won't be broken.

@PaulL1
Copy link
Contributor Author

PaulL1 commented Oct 23, 2014

Have discussed with @c0bra and @swalters, and we're OK with this as a way forward, so merging.

PaulL1 added a commit that referenced this pull request Oct 23, 2014
Fix #1792(options): use gridOptions rather than copy them
@PaulL1 PaulL1 merged commit 67b08d8 into angular-ui:master Oct 23, 2014
@c0bra c0bra removed the in progress label Oct 23, 2014
@PaulL1 PaulL1 deleted the options_watches branch March 15, 2015 02:32
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.

2 participants