Refactor/extract functions #13
Conversation
There was a problem hiding this comment.
Nice refactoring @uqmessias! It's looking great. I left some questions.
| }, | ||
| }; | ||
|
|
||
| export const renderSvgElementByNodeWithItsChildNodes = ( |
There was a problem hiding this comment.
what do you think of adding tests for this function in order to make sure each element has the expected result for the allowed attrs?
There was a problem hiding this comment.
That's a great idea, but before we do that, let me see if I understood it correctly.
What would be the purpose of these tests? To make sure that the function is rendering only the allowed attributes or to ensure that each element has the expected allowed attributes?
I'm asking that, because depending on the answer, the implementation will be different.
There was a problem hiding this comment.
What would be the purpose of these tests? To make sure that the function is rendering only the allowed attributes or to ensure that each element has the expected allowed attributes?
☝️
yeah, this function with the mapping above seems to be the core of this library. So having tests for it would would give more confidence for future maintenance.
There was a problem hiding this comment.
I was reviewing your comment about testing this function and realize it already has its tests, besides that, the functions on the map you mentioned has tests to ensure each element is being rendered properly.
Do you think we still need more tests? If so, what do you suggest to test?
uqmessias
force-pushed
the
refactor/extract-functions
branch
from
4a5ea9e to
a5d2c3b
Compare
This is a big step before converting the projet to Typescript and solves #11