Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

feat(ErrorLogger): accept object instead array - I137 #165

Merged
merged 1 commit into from
Oct 14, 2019
Merged

feat(ErrorLogger): accept object instead array - I137 #165

merged 1 commit into from
Oct 14, 2019

Conversation

flagoon
Copy link
Contributor

@flagoon flagoon commented Oct 11, 2019

Signed-off-by: Pawel -Muody- Kochanek [email protected]

Issue #137

ErrorLogger was accepting an array of objects and there was possible issue with duplicate errors. The task was to change array to object.

Changes

  • change 'errorComponentGenerator' function. Right now it's mapping over keys of error object, not array as before.
  • change propTypes, not errors are required object, not array
  • change error value in tests
  • in action.js, instead errors.length, we use Object.keys(errors).length.

Flags

  • There were not many changes I made, main change was iteration over Object.keys instead of errors array. The rest was written such a way I just made some fine tuning. Also in tests I just change the errors object structure. After that, all errors passed.

@flagoon
Copy link
Contributor Author

flagoon commented Oct 12, 2019

This is perfectly correct but if you map directly over Object.entries then you have directly the key and the value. So you don't need to access your map to get the value :

Object.entries(errors).map(([key, value]) => ... )

Nice trick with destructuring the values in.map(). I decided not to use entries and I didn't like to call variable[0] and variable[1]. I'm just wondering, if I should use key for react component key, or just value? If I use key, I would have to change keySwitchCase function. If I use value, then I have eslint error about key variable, that is not used.

@Ph0tonic
Copy link

You are right, in fact you just need the values so you can simply use the Object.values(errors).map(error => <ErrorComponent ... />) and then use this value for the key like before. In this way you won't have any eslint error about unused variables.

@flagoon
Copy link
Contributor Author

flagoon commented Oct 13, 2019

You are right, in fact you just need the values so you can simply use the Object.values(errors).map(error => <ErrorComponent ... />) and then use this value for the key like before. In this way you won't have any eslint error about unused variables.

Thanks for this. I corrected my code and now it's more readable and easier to understand.

@jolanglinais jolanglinais added Difficulty: Medium Hacktoberfest by DigitalOcean and DEV Type: Enhancement ✨ Improvement to process or efficiency labels Oct 14, 2019
@jolanglinais jolanglinais changed the title feat(ErrorLogger): ErrorLogger accepts object instead array #137 feat(ErrorLogger): accept object instead array - I137 Oct 14, 2019
Copy link
Member

@jolanglinais jolanglinais left a comment

Choose a reason for hiding this comment

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

This looks good to me. Great additions! I'm going to merge this in and we can iron out any possible bugs through #167

@jolanglinais jolanglinais merged commit 73f73fe into accordproject:master Oct 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Difficulty: Medium Hacktoberfest by DigitalOcean and DEV Type: Enhancement ✨ Improvement to process or efficiency
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants