search

LocusEnergy / ember-sparkles

Collection of composable D3 components built with ember-d3-helpers
http://locusenergy.github.io/ember-sparkles/#/line-chart
MIT License
56 stars 9 forks source link

Updates all templates to use ember-let instead of `with`. #51

Closed patrickberkeley closed 7 years ago

patrickberkeley commented 7 years ago

@taras this is to address: https://github.com/LocusEnergy/ember-sparkles/issues/42.

Let me know if you were looking for anything else there.

taras commented 7 years ago

We're going to start working on this soon and we'll get this merged in. Thank you for doing this :)

ghost commented 7 years ago

@patrickberkeley this looks great! What about using inline let where we can?

patrickberkeley commented 7 years ago

@zigahertz would you mind commenting with an example? I'm not seeing attributes in the components that could be eliminated with let.

ghost commented 7 years ago

@patrickberkeley I meant, using the inline let as described here - https://github.com/thefrontside/ember-let

Not to eliminate attributes, just to clean up the templates more (it allows for one less level of indentation)

patrickberkeley commented 7 years ago

@zigahertz gotcha. I've added a commit that uses the inline syntax: https://github.com/LocusEnergy/ember-sparkles/pull/51/commits/8d3f52ec7b572dcfcebfe32c08ecad35dd99db9c

Note, I needed to use version 0.4.0 of ember-let in order to get this working. With anything newer, I'd get errors on this line. It looks like somehow ember-let thinks I'm using the block syntax.

ghost commented 7 years ago

@patrickberkeley interesting, thanks for trying this out... @taras as one of the authors of ember-let, any thoughts on this?

patrickberkeley commented 7 years ago

@zigahertz I merged in your recent updates and gave ember-let 0.5.3 another shot. I get a different error now, so still no 🎲🎲 . The inline let variables seem like they aren't being defined because – for example – the compute helper complains TypeError: Cannot read property 'apply' of undefined here.

yarigpopov commented 7 years ago

@patrickberkeley feels like something incompatibility with ember v2.10 I have the same error for grouped-chart

ghost commented 7 years ago

@patrickberkeley @chilicoder I see - @taras and I did some investigation a little while ago - we think the issue lies exactly where you pointed, at https://github.com/LocusEnergy/ember-sparkles/pull/51/files#diff-96c3eb164476c32c443d1119307c3c1fR25.

I think the solution is to upgrade the library to use Ember 2.10. This breaks because of the compute logic for the scale functions, it's too nested and needs to be refactored. This is where let in the parent scope might be quite handy. I am (mostly) on vacation until the second week of January, if I get some free time I will try to outline my idea more clearly and push a PR. Otherwise, if this makes sense - please feel free to give it a shot. If not, please feel free to ask questions here or send me a DM in the Ember community Slack.