Skip to content

Conversation

@ptitFicus
Copy link
Collaborator

Here is what I did for #49
I tried to keep the behaviour of error as close a possible from the current print behaviour.
There is a few things I'm not 100% satisfied with :

  • There is huge logic duplication between ErrorCross and TailSpin (and their tests) : we may want to factorize some code
  • The isInState function in core.jsx is not super clear

Any suggestion / improvement / fix appreciated :)

@fabienjuif
Copy link
Member

Great work, once again ;)
My first comment, I read it fast :

  • I think the isInState name is not great indeed, at first I was expecting that you will use react state
  • I agree with you about TailSpin + ErrorCross code factoring
  • I think a test is missing : there is error prop test ?
  1. The component is printed with default loader parameters
  2. The error prop is set to true
  3. The Cross should be printed

@fabienjuif
Copy link
Member

@ptitFicus I close the PR since:

  1. It is old now
  2. There is a lot of conflicts

Feel free to reopen, and sorry about that :(

@fabienjuif fabienjuif closed this Nov 2, 2017
@ptitFicus ptitFicus mentioned this pull request Nov 4, 2017
@fabienjuif fabienjuif deleted the errorIndicator branch November 13, 2017 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants