Code review - Issue 8186045: Add an extension for managing overlay indicatorshttps://codereview.appspot.com/2013-04-01T15:04:57+00:00rietveld
Message from unknown
2013-03-30T20:30:03+00:00rhardingurn:md5:3fadb997a31aab158e1f6dfd5e8c2dcb
Message from rharding@mitechie.com
2013-03-30T20:30:05+00:00rhardingurn:md5:525f0622cf135696b567d64284573826
Please take a look.
Message from rharding@mitechie.com
2013-04-01T11:40:21+00:00rhardingurn:md5:9da15fa2e0382720641adc4683c490a4
On 2013/03/30 20:30:05, rharding wrote:
> Please take a look.
Jeff brought up that "my only concern would be that sometimes yuiid's change"
If this is the case, the worst case scenario is that we create a new Indicator instance for a node that already had one in the past. Since all of them are logged into _instances, they will all get cleaned up though. Since the extension is tied into Views, it makes sure that each time a view is destroyed all Indicators are cleaned up.
Message from jonathan.sackett@canonical.com
2013-04-01T12:36:12+00:00j.c.sacketturn:md5:665e5a94050fc2efdf50b4a60a384f82
Rick, one comment in the diff, and a question about your worst case.
> If this is the case, the worst case scenario is that we create a new Indicator
> instance for a node that already had one in the past.
Are we certain we don't enter a circumstance where we can't unhide an element, b/c the yuid for the node has changed making it impossible to grab the right one and call .success on it? I'm completely unfamiliar with the circumstances under which yuid's change--if this is impossible, just let me know.
https://codereview.appspot.com/8186045/diff/1/test/test_overlay_indicator.js
File test/test_overlay_indicator.js (right):
https://codereview.appspot.com/8186045/diff/1/test/test_overlay_indicator.js#newcode169
test/test_overlay_indicator.js:169: it('indicator manager wires into an object properly', function() {
Should these be in a separate describe block? They're testing a different, though related, object from the overlay indicator.
Message from rharding@mitechie.com
2013-04-01T12:39:49+00:00rhardingurn:md5:f9e7fece199bff98adb5f05874397210
On 2013/04/01 12:36:12, j.c.sackett wrote:
> Rick, one comment in the diff, and a question about your worst case.
>
> > If this is the case, the worst case scenario is that we create a new Indicator
> > instance for a node that already had one in the past.
>
> Are we certain we don't enter a circumstance where we can't unhide an element,
> b/c the yuid for the node has changed making it impossible to grab the right one
> and call .success on it? I'm completely unfamiliar with the circumstances under
> which yuid's change--if this is impossible, just let me know.
I'm still looking into it myself. I wasn't aware of the issue and haven't run into it. If this is true and an issue you're right. I'll adjust the api of this so that when you call showIndicator it returns the reference of it. Then the ajax callback will need that to hide the indicator again.
var indicator = this.showIndicator(node);
this.get('store').someajaxcall(charmID, {success: function(data) {indicator.success()}...
>
> https://codereview.appspot.com/8186045/diff/1/test/test_overlay_indicator.js
> File test/test_overlay_indicator.js (right):
>
> https://codereview.appspot.com/8186045/diff/1/test/test_overlay_indicator.js#newcode169
> test/test_overlay_indicator.js:169: it('indicator manager wires into an object
> properly', function() {
> Should these be in a separate describe block? They're testing a different,
> though related, object from the overlay indicator.
I debated. It needs much the same YUI setup so I didn't create an extra block. I can split that up and dupe the setup.
Message from jonathan.sackett@canonical.com
2013-04-01T12:45:22+00:00j.c.sacketturn:md5:b769dbf9c0152f49d261ae28a8845800
On 2013/04/01 12:39:49, rharding wrote:
> I'm still looking into it myself. I wasn't aware of the issue and haven't run
> into it. If this is true and an issue you're right. I'll adjust the api of this
> so that when you call showIndicator it returns the reference of it. Then the
> ajax callback will need that to hide the indicator again.
>
> var indicator = this.showIndicator(node);
>
> this.get('store').someajaxcall(charmID, {success: function(data)
> {indicator.success()}...
Sounds good, thanks.
> I debated. It needs much the same YUI setup so I didn't create an extra block. I
> can split that up and dupe the setup.
No actually, you're right. If they share the same setup, it probably shouldn't be broken up. This is my internal pragmatist fighting against my internal idealist. But the pragmatist wins.
Message from jeff.pihach@canonical.com
2013-04-01T14:20:59+00:00jeff.pihachurn:md5:b1128d1b1d7e82825e9d331d61a3049d
LGTM go....extensions!
Message from unknown
2013-04-01T14:59:14+00:00rhardingurn:md5:81a2634c36dc710de99928efb1ad830e
Message from unknown
2013-04-01T15:02:22+00:00rhardingurn:md5:4ae8b1bcc97da209fd3a33c338baf7a6
Message from rharding@mitechie.com
2013-04-01T15:04:57+00:00rhardingurn:md5:d9689074ec38ed45fb217c0083564ed9
*** Submitted:
Add an extension for managing overlay indicators
- Add an extension used in views that use an overlay indicator
- Tracks instances, makes sure they're not duplicated
- Makes sure on the view/objects destroy it cleans out the indicator
instances.
- Add tests to verify it does the job of preventing dupes and destroying.
R=j.c.sackett, jeff.pihach
CC=
https://codereview.appspot.com/8186045