decomposed level bubbles

[cforkish]

this is a proposal for a decomposed design pattern for components. the initial motivation was to make it easy to find and select "subcomponent pieces" of the main component in unit tests. however, upon further reflection, i think the decomposition itself is a design improvement in its own right, because it makes it easier to see what will get rendered by different code paths, and will hopefully encourage more code reuse.

i went with a convention of using function components for all the subcomponents as a way to enforce the idea that subcomponents are "pieces" of a component -- i.e. if it would need to be a class component, it should probably be its own component in its own file. that said, i didn't realize that our linter would enforce proptypes on function components. given that, it might look cleaner to use class components. but i kind of like this distinction of components=classes, subcomponents=functions.

i would like to point out that the code didn't actually change much with this update -- it mostly just moved out of the component class and into the module. most of the subcomponents i created were already in their own methods anyway, so it mostly just mean turning those methods into function components. decomposing rendering into multiple methods on component classes is already a very common design pattern in our codebase, so this change mostly amounts to a tighter encapsulation of that decomposition.

ProgressTableLevelBubble is an extreme example of this pattern because it has so many subcomponents for all the different bubble configurations. most components would probably only have a few subcomponents at most. for a simpler example, see LessonArrow in this PR.

would love to get others' thoughts on this. i like it, but i can see how it might not be for everyone! :)

questions:

1. do you think it improves the tests at all?
2. do you think it makes the component code easier or harder to read (or neither)?

[maureensturgeon]

I like the different bubble types factored out, I think that makes sense to me. This wrapper doesn't seem super valuable outside of testing, especially since it's only used in one place. IMO the code would feel more readable if we just wrapped the return in a link.

[cforkish]

see my comment below in the tests and lmk what you think

[maureensturgeon]

Are these bubbles used in other places? I'm wondering if they should just be their own components

[cforkish]

currently they are not used in other places. however, we plan to refactor our existing ProgressBubble component (that this is based on) so that we don't have two versions. i haven't started looking at what all that will involve, but its easy to imagine that we might want to build it out of these subcomponents instead of building it on top of this whole component, at which point we'd want to move these out into a common module.

[cforkish]

@maureensturgeon re. your comment above about the wrapper -- imo this is a big enough improvement for this test to warrant it being its own subcomponent. it allows for the possibility of the link implementation changing without breaking this test. more to the point though, i'm just not a fan of the is('div') check because i don't think that's necessarily telling us what it purports to be verifying.

it's also not hard for me to imagine LinkWrapper evolving into something that we'd want to reuse (e.g. built-in analytics or url validation or something).

unless you think it makes the code harder to read, i'm in favor of keeping it. but lmk if you disagree.

[cforkish]

hmm, i guess i could just do a find('a') and verify the length is either 0 or 1. that would satisfy me if you want me to remove LinkWrapper.

[maureensturgeon]

I could also imagine it evolving, so that makes sense to me. Would it be worth making a LinkWrapper component now and putting it somewhere common so we can start using it?

[jamescodeorg]

After looking at the for a bit, I'm afraid I like the original version better.

I'm new to React and learning the idioms so take my comments with a grain of salt, but the sub-components seem too small to me (similar to how a very short function might obscure the logic rather than clarify it).

I think the sub-components could still be useful if they represented a reusable concept or if they abstracted away some interesting logic, but I'm not sure that's the case here? One test that I often apply to determine if an abstraction (e.g. class) should exist is: when reading code that uses this abstraction for the first time, would someone be able to make some assumptions about how it works and gloss over the details of its implementation? Taking SmallCircle as an example, when I got to line 211/216, I see that I'm instantiating SmallCircle, but it takes all the props and I still feel like I need to read the implementation of SmallCircle to know what it does. In this way, it doesn't feel more abstracted than renderSmallBubble().

The main benefit, as you say, is that you can assert that the sub-component exists in the unit tests. I wonder if the test would be almost as good by asserting the style of the contained div. This could be done in a shared function to provide some abstraction?

Anyway, we can chat more on Tuesday. I don't think the new version is so unreadable that I would block this change, but my initial impression is that the original version was a bit more readable.

[jamescodeorg]

Why do we need two levels of <div>s here?