Stef's code review about ember-cli-flash HOT 5 CLOSED

@truenorth Cool, let me know how I can help! :)

from ember-cli-flash.

this project looks great, really nice!

This pass is mostly superficial, I will take some time tomorrow to use this in an app, and give better feedback.

I'm leaving some my comments in the form of todos, so that myself or someone else interested can make the appropriate updates :)

• https://github.com/poteto/ember-cli-flash/blob/develop/addon/components/flash-message.js

  run.scheduleOnce('afterRender', this, () => { the onceness is detected based on the this and the function, but in this case the function is recreated on each call, so once can never happen

• many of the computed should actually be marked as readOnly

  • https://github.com/poteto/ember-cli-flash/blob/develop/addon/components/flash-message.js#L39-L47
  • https://github.com/poteto/ember-cli-flash/blob/develop/addon/components/flash-message.js#L32
  • https://github.com/poteto/ember-cli-flash/blob/develop/addon/components/flash-message.js#L20
  • https://github.com/poteto/ember-cli-flash/blob/develop/addon/components/flash-message.js#L18
  • https://github.com/poteto/ember-cli-flash/blob/develop/addon/flash/object.js#L13-L20
  • https://github.com/poteto/ember-cli-flash/blob/develop/addon/services/flash-messages-service.js#L21-L28

• missing super

  • https://github.com/poteto/ember-cli-flash/blob/develop/addon/flash/object.js#L28

• state on prototype (these should likely be moved to init)

  • https://github.com/poteto/ember-cli-flash/blob/develop/addon/services/flash-messages-service.js#L18

• funky indentation: https://github.com/poteto/ember-cli-flash/blob/develop/addon/services/flash-messages-service.js#L77

• skipped tests https://github.com/poteto/ember-cli-flash/blob/develop/tests/acceptance/integration-test.js#L24-L45, likely missing andThen

from ember-cli-flash.

Thanks for the review @stefanpenner!

• run.scheduleOnce('afterRender', this, () => { the onceness is detected based on the this and the function, but in this case the function is recreated on each call, so once can never happen
• many of the computed should actually be marked as readOnly
• missing super
• state on prototype (these should likely be moved to init)
• funky indentation: https://github.com/poteto/ember-cli-flash/blob/develop/addon/services/flash-messages-service.js#L77
• skipped tests https://github.com/poteto/ember-cli-flash/blob/develop/tests/acceptance/integration-test.js#L24-L45, likely missing andThen
  • related – #32

from ember-cli-flash.

Thanks for making all of these improvements @poteto. We really appreciate it!

from ember-cli-flash.

👍

from ember-cli-flash.