Refactor web and library components

[wlthomson]

A general issue for a few things which I think could do with some tidying up (good to get discussion/agreement on these patterns early, as otherwise might start some anti-pattern trends we regret later!).

Usual disclaimer that below is feedback, know I'm late to the party 🎉 and haven't communicated intended design conventions too well, so on me and not intended as criticism!

1. Header and Footer.

Probably happened when I wasn't looking, so my mistake on this one. At some point these got extracted out to web components. As far as I know, these components are only used once and in one place, so I'm not sure making them components gives us much!

I'm not 100% opposed to extracting these out, but at this stage I think the motivation is to tidy up the code (given away by the fact that the components have no props except for classes), which I think is probably not a good reason to make something into a component? In general, application components should only be templates, anything more granular should be in the ui library. If we find ourself trying to make molecules etc. which are too specific for the library, then that's probably an indication of a code smell to be investigated. Solution for now is I suggest we squash these back into App.tsx (would also simplify the styles a bit)?

2. TMF/UC icons.

These are super tightly coupled to the web app, so should probably have the svgs under web/src/assets or something (and maybe add SvgIcon to ui library... UPDATE: we already have this!) rather than as "hardcoded" components in the shared library?

3. EntityTypeFilter.

I think this is a bit over-engineered (I don't think this component has any use outside EntityBrowser)? Probably can just go with a ToggleButtonGroup directly in the EntityBrowser. Simplifies things a bit?

4. EntityBrowser

I think a few changes made to this component which strike me as possibly done in a bit of haste (again, not intended as criticism). I think the new callbacks are good, and create a nice consistent API... but I think a few of the non-callback props could be refactored a bit, e.g. noResultsMessage breaks the component design patterns we've been using a little bit (i.e. that feedback is handled up the component tree via callbacks). Also the variables prop (I think possibly a trend I started, so my bad on this one) is a bit generic and should probably be split out into its constituent keys (react encourages pure props, POJO props are dangerous!).

Expected Behaviour

I think main feedback here is that there is a bit of over-engineering (I think motivated partly by wanting to extract out big/messy component definitions) which has/may have the effect of making the code more tricky to read/follow (I think this happened a bit with styles, too). It's something I'm guilty of a lot too, so it's good to remember the rule of YAGNI (fun to say, too).

• All components/assets specific to UC in web project (there's a bit of leeway here for stuff like Entity etc., but the more tightly coupled something is to UC, the more it should probably go in the web project, I think?)
• All components smaller than template in library.
• Avoid components created just to reduce the size/messiness of code (rule of two, if part of a component definition isn't going to be used more than one place, probably should be defined inline rather than extracted into it's own component). Tackle code complexity with new library sub-components rather than big one-off app components.
• Avoid passing props as POJOs, make props as pure as possible 👍 .

Having said all the above, the functionality itself seems to be working well 😄.

Current Behaviour

See above.

Possible Solution

See above.

[mark-prins]

Thanks Will! and yes - agree with the intention and the changes as abstract proposals.

Low priority for implementation though - given the size of this app and intended usage. Would like to have something functional first and then more refactoring after. I disagree with the comment that changes were made in haste, I think it was more misunderstanding than any push or time constraint. Thanks for laying out the intention more clearly here, that should help with the next steps

Disagree about having a very big App.tsx though - which is why the header, footer, entity type filter were created. Would like to discuss that point further.

I also don't like having a large number of props on components, and thought that using an interface and passing an object would be ok. Happy to go with the flow on that one.

[wlthomson]

Low priority for implementation though - given the size of this app and intended usage. Would like to have something functional first and then more refactoring after.

Fair enough, just thinking about getting agreement moving forward so we can avoid as much technical debt as possible!

I disagree with the comment that changes were made in haste, I think it was more misunderstanding than any push or time constraint. Thanks for laying out the intention more clearly here, that should help with the next steps

Yeah, sounds like it was a communication error on my part. I assumed there was a rush because of the push for getting something functional... but I was offline for a couple of weeks, so that's probably on me (sorry about that).

Disagree about having a very big App.tsx though - which is why the header, footer, entity type filter were created. Would like to discuss that point further.

I agree that we can extract out these components, but since they have no re-usability, declaring them as stand-alone files isn't going to give us much apart from potential headaches (IMO). I think a good compromise could be to declare them separately but in App.tsx. If we can declare them as reusable UI components with props, then I would be completely behind making them stand-alone components in the ui lib!

Re. EntityTypeFilter however, I think that component is probably unnecessary (since it's quite a small wrapper and is very tightly coupled to the Explorer). If we separate it out into a reusable button group type component, that would be fine in my books (but I think this is already covered by ToggleButtonGroup). So, my thinking on this is still to get rid of it for now, but I agree on returning to it if it becomes big enough to be its own component (but, again as a presentational component which takes props which we can feed to it from its parent or from redux).

TLDR; all for splitting out presentational components into the ui library, but splitting out a component declaration into different files seems more cosmetic rather than composition/modularisation, which I think is a bit of a code smell/anti-pattern (IMO it makes for less code in App.tsx, but still the same amount of code in total, just more files to cross-reference for the coder to understand what's going on)!

I also don't like having a large number of props on components, and thought that using an interface and passing an object would be ok

I've done the same in the past, and think the motivation is good, but some good practices for OOP/procedural programming go bad because of reacts "hierarchical" composition model. Just from looking at components from popular libraries/frameworks (e.g. RN, material UI), I've seen it's often the case that higher up components end up with lots of props... and I think that's generally accepted as a cosmetic yuckiness that has to be lived with due to how react works (grouping props into POJOs can cause unexpected render bugs/performance issues due to object mutability).

TLDR; similar to previous, I think it's one of those cosmetic sacrifices that comes with reacts compositional design, as the cosmetic niceness will likely cause more of a headache than benefit. However, if you feel we are getting too many props in the parents, I think one solution could be to consider extracting them out via children or explicit component props?

[mark-prins]

Sounds good - and let's go with lots o' props and see how it goes. I expect that it'll be fine, as with the App.txs

[wlthomson]

A lot of stuff has been done in the PR for #190.

Next steps are to hook those changes up with the web app:

• Organise redux state by components (similar to the hooks in the EntityBrowser story).
• Wrap components in connect and feed props directly from redux state (once this is done, I think it will be clearer why all this was worth it!).
• Update templates to follow atomic design principles, should take components as props and just specific a general page layout, e.g. right now we should only have one template, e.g. Page.tsx (or something a bit more specific, but point is that it's a generic page that just describes the layout, not the contents!). Page can then take some component props, e.g. headerComponent, pageComponent, footerComponent. As an example, when viewing the explorer, pageComponent updates to the EntityExplorer (off-road, I think Explorer is a bit generic), when viewing details, updates to EntityDetails (or whatever the name is).

All the above will make stuff a lot simpler going forward... I think definitely worth it as I'm convinced it'll save us lots of time which would otherwise be spent trying to figure out how to organise the store/how to split state between components etc. (basically the store just stores the state for the components, which connect directly or get their props from a parent who connects directly). It also simplifies the dgraph stuff, as everything can be accessed via reducers/sagas (e.g. if search button is pressed, the store already contains the search term from the SearchBar, the filter state from the ToggleBar, the page from the TablePagination, the ordering from the Table etc. etc... the reducer/saga can access all those as well as update them all in one go!).

@katherine-sussol @mark-prins Does that make sense? @katherine-sussol may want to take this into though when designing the details page (but don't worry too much, I can refactor as we go).

[mark-prins]

yep, makes sense. would like to look at other options for styling rather than passing classes and relabelling as it filters down. and dropping material-ui theming; is that required? question is, why use material-ui if not using the stying part of the library?

Also, the passing of components as props - needed? why not compose at a higher level?

[wlthomson]

yep, makes sense. would like to look at other options for styling rather than passing classes and relabelling as it filters down. and dropping material-ui theming; is that required? question is, why use material-ui if not using the stying part of the library?

Also, the passing of components as props - needed? why not compose at a higher level?

I think the styling issue has been addressed in #205 (I think that issue was created since this comment was made)?

Re. the passing of components as props, I see quite a few advantages in terms of flexibility and ease/reuse of composition.

The main thing component props bring is enabling us to define "high-level" (i.e. higher up the component tree) components, without sacrificing any control of the consuming code over the child components. This gives full flexibility, and I think simplifies a lot of things as it removes the need to design component "APIs" (by making components purely presentational, and exposing all the children as props, you already have all functionality exposed!)

For example: if we export an EntityBrowser with the search bar, toggle bar, table etc. all built in, I think we will end up either having to restrict the control of the consuming code over the styling/props given to the child components (i.e. accepting that the app code won't be able to style or pass them props directly), or having to come up with an API which allows the child props to be accessed via the parent props (which often involves nested objects and styles etc. which don't scale beyond a few levels). I think the "component as props" model gives full control, while also exposing a nice tidy high-level component. By exposing the components directly, it also allows for easily creating new components on-the-fly by combining different compositions together and with different component props.

From the point of view of the app vs. lib separation, I think having library components use component props makes things a lot easier, because it removes any need to add state/styles to them. Instead, the app can define its own simple "wrapper" components which connect the library component to the apps context (e.g. redux or theme etc.). That way we can basically export the UI using library components, without worrying about the context the component will be used in (no need to worry about navigation or styles or anything like that) as we can inject everything in the app. We can then still use the library high-level components, but with our "super powered" components which are aware of the app state and styling. I've also found that this makes designing components a lot easier, as it decouples them from any complexities involved in the app.

Does that make sense or is that still not answering the Q?

[mark-prins]

Does that make sense or is that still not answering the Q?

Makes sense. Doesn't answer the question :)

I get where you're at with components-as-props. The question was, why have a composed version in the library, which is then used in the app when the app is passing in each individual aspect as component props. Why not have the composed component only in the app?

I can see that it allows the composed component to be viewed in the lib storybook. Other than that?

[wlthomson]

Going to close this out as outdated as of #201.