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.

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

Make App's showView() more consistent when working with new vs. existing views. #436

Open
ericf opened this issue Feb 5, 2013 · 5 comments

Comments

@ericf
Copy link
Member

ericf commented Feb 5, 2013

App's showView() method can take a view name or an existing view instance. A problem arises with ad-hoc attributes in the following scenario:

var view = new Y.View();
Y.log(view.template); // => ""

app.showView(view, {template: 'bla bla'}, {update: true}, function (view) {
    Y.log(view.template);        // => ""
    Y.log(view.get('template')); // => "bla bla"
});

When passing an existing view instance with the update option set, the showView() method will call setAttrs() on the view instance with anything in the config argument. This means that a template attribute is created.

Compare the above with calling showView() using a named view:

app.showView('someView', {template: 'bla bla'}, function (view) {
    Y.log(view.template);        // => "bla bla"
    Y.log(view.get('template')); // => undefined
});

The problem is that non-ad-hoc attributes specified in a view class' _NON_ATTRS_CFG property only apply at construction time.

Some possible solutions:

  1. Simply document this as an expected difference in behavior when working with existing vs. new views.
  2. "Filter" out properties which match items in _NON_ATTRS_CFG when showView() is updating an existing view.
  3. Apply the above filtering, but also assign the filtered properties as new values to properties on the existing view instance.
  4. Remove showView()'s update option, and let people who are managing their own view life cycles, also manage updating their view's properties and attributes.

I don't see a great solution above to address this. I'm open to hear other ones and/or which of the above you like best.

@rgrove
Copy link
Contributor

rgrove commented Feb 5, 2013

I vote for option 4. update adds unnecessary magic.

@hatched
Copy link

hatched commented Feb 5, 2013

While removing functionality that people might be using could cause some headaches for some I agree with @rgrove

@ericf
Copy link
Member Author

ericf commented Feb 5, 2013

@hatched If I go with option 4 I would first deprecate the feature, log a warn if it is used, and remove it from the documentation. Then during some future refactor and in a future release I would remove the code which implements it.

@tivac
Copy link
Contributor

tivac commented Feb 5, 2013

I use update extensively, but always to set attributes so I've never run into this behavior. It's a very useful convenience, one that I'm pretty sure I lobbied for when I first started using Y.App. I'd hate to see it go.

Issues like these are exactly what made me nervous about ad-hoc attributes in the first place. Passing an object in & having some things be attributes & some not (but only at construction time) depending on the contents of a protected static array isn't terribly intuitive.

I vote option 1

@jimwhimpey
Copy link

I vote for option 4. update adds unnecessary magic.

Agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants