Major refactoring: what should be improved?

[iamakulov]

One of my goals here is to resolve all the major code issues so that other contributors can get involved easier. Here’s what I’ve got so far:

• [x] Button icons are placed separately from button descriptions (svg.js vs services/facebook.js, services/linkedin.js, etc). It would be easier if the icon was provided in the same file where the button description is placed.
• [ ] A signature like this:

  popupUrl: 'https://pinterest.com/pin/create/button/?url={url}&description={title}',

  is unclear because you don’t understand where title comes from and what are the other parameters you can use. I’m going to replace such strings with functions and explicitly pass parameters there.

• [ ] SVG icon strings encode SVG paths but omit the M letter in the code. This causes issues when contributors submit PRs providing this button (e.g. #101)
• [ ] Generally, there’re some code pieces that aren’t used for anything (e.g. this.number in widget.js) and should be removed.

I’m sure I’ll discover more points during the refactoring. Meanwhile, @vitkarpov, @ilyabirman, do you have any considerations about this or things that should be also improved?

[vitkarpov]

+1 for everything. I'd suggest unit tests and code coverage (can take this). Unit tests can be very helpful on transition to a new major version (can illuminate a lot of subtle breaking changes).

[iamakulov]

What parts of code are you suggesting to cover with unit tests?

[vitkarpov]

Obviously it should be public API (which is small, and it's good), but we have to be sure that there's no dead code: some internal parts which haven't been called during the public API test

On Sun, 22 Jan 2017 at 12:39, Ivan Akulov notifications@github.com wrote:

What parts of code are you suggesting to cover with unit tests?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ilyabirman/Likely/issues/120#issuecomment-274320157, or mute the thread https://github.com/notifications/unsubscribe-auth/AC3Nv7cD4SYzndAmn-95xX1Z-s03-oR7ks5rUyPUgaJpZM4LqDVS .

[vitkarpov]

Moreover, there're some independent modules with clear public API, for instance, button

[iamakulov]

I don’t know. I mean, unit tests will add the infrastructure complexity (as well as the UI tests also added it – look how much time we’ve been spending on them), so I don’t want to opt in for them unless they have a value. And I don’t see a real value at the moment.

If you have any examples of something that we should use unit tests for and that can’t be done or is too complex for doing with UI tests, I’m absolutely open for discussing it. Otherwise, I wouldn’t spend time on them.

[vitkarpov]

Yeah, I know what you're talking about. Let's not to make it more complex for now.

[NikolayRys]

It is now handled as part of 2.5 and 3.0 releases.