Skip to content
This repository was archived by the owner on Dec 4, 2017. It is now read-only.

Conversation

@adaojunior
Copy link
Contributor

No description provided.

@adaojunior
Copy link
Contributor Author

I messed up in the other pull request

@kwalrath
Copy link
Contributor

It looks like you haven't incorporated all the comments from #696, so I'll wait for another commit before taking a final look.

@adaojunior
Copy link
Contributor Author

@kwalrath I removed the unused file and changed to relative imports.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any feedback on this string interpol style yet?
The Dart style guide discourages {} here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zoechi Aparently they're only necessary when it is a expression or you're calling a method in a object. I'll remove them. Thanks

@kwalrath
Copy link
Contributor

@sanfordredlich could you please do a technical review? I'm especially curious about the _tick() => new Future(() {}) code in heavy_loader_component.dart. (It's derived from the TS code, but using a Timer resulted in more complicated code, so I suggested new Future() as a way of kicking the event loop.)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "JavaScript cycle" a good explanation here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. How about "browser event loop"? We should perhaps make this change to the JS/TS samples, too.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good and should fly in JS land as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, also sent a PR to change it in the TS example.
#710

@kwalrath kwalrath self-assigned this Jan 20, 2016
@kwalrath
Copy link
Contributor

I'm going to merge this PR, so we can point to this code from a placeholder page. We'll do a final technical review before I write & publish the real page.

@kwalrath kwalrath closed this in 6ee3756 Jan 21, 2016
@adaojunior adaojunior deleted the structural-directives branch January 21, 2016 23:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants