Components should not depend on configuration objects

[bertrandmc]

Hi,

Thanks so much for this project, we are looking forward to integrating it in the XGovFormBuilder project.

I have had a look at the components created so far and I would like to raise an issue with their design.

React components are best if they are written in a way that, on the user-land, we can consume and build the interface as pure JSX instead of passing configuration via objects, there are several reasons for this but the most important is to make components easy to extend and customize for different use-cases without need to implement and document APIs.

Below I am giving an example using the footer.

ATM this is how I can write the footer with navigation.

import { Footer } from "@hods"

return (
  <Footer
    navigation={[
      {
        href: "/feedback",
        text: "Feedback",
      },
      {
        href: "/help",
        text: "Help",
      },
      {
        href: "https://gov.uk/",
        text: "Gov.UK home",
      },
    ]}
  />
)

As a user, if I needed to give some prop to one of the menus, let's say a className or a test-id, the component have to expose that API via the configuration object and I will have to be diving into the documentation to make sure it does, otherwise I can't extend it, also, if I want to handle some UI condition, let's say hide one of the menus based on some criteria, I have work on the object/JS instead of JSX, so the design is limiting the tools I have.

Ideally, this is how I would like to write the component in JSX:

import { Footer } from "@hods";

const { enableFeedback } = props;
const { Navigation } = Footer;

return (
  <Footer>
    { enableFeedback && <Navigation href="/feedback" data-test-id="123">Feedback</Navigation> }
    <Navigation href="/help" className="active">Help</Navigation>
    <Navigation href="https://gov.uk/">Gov.UK home</Navigation>
    <ApplicationVersion />
  <Footer>
)

Because passing props to a component is the expected behaviour, I don't need to remember what the navigation list should look like nor go through documentation, I just pass the props I need and job done, from your side, there is no need to implement/handle that via the configuration object nor write documentation for it.

[RobinKnipe]

Hi @bertrandmc - you're right, the nav list should be provide in the more common and better supported way (i.e. through well structured JSX elements, provided as children), rather than as config. This implementation of the footer component is a legacy one that is in need of an update. As part of the evolution of the base components, we will favour the design towards encouraging and facilitating the writing and production of well structured semantic markup - we're just not quite there yet...

[daniel-ac-martin]

Hi @bertrandmc. Thanks for your thoughts on this.

I think I disagree with this proposal. I think that in the case of lists it is reasonable to use a JS array as that is better defined than the HTML style. (You could pass in anything with the HTML style not just list elements.) I remember reading that Facebook follows this practice as well (rather then using composition for this). (I should try to find the blog post about that...)

Incidentally, overuse of composition was one of the reasons that I grew to dislike the govuk-react library.

To illustrate the point, consider that the Footer has multiple menu's within it. Which menu should the children go into? How would one place elements into the other? (@RobinKnipe: \<ping>.) (Update: Just checked and the HODS one only has one menu in it but it does still have content outside of that menu.)

In practice I think you would need an interface more like the following:

  <Footer
    navigation={<Fragment>
    { enableFeedback && <Navigation href="/feedback" data-test-id="123">Feedback</Navigation> }
    <Navigation href="/help" className="active">Help</Navigation>
    <Navigation href="https://gov.uk/">Gov.UK home</Navigation>
    </Fragment>}
  >
    <ApplicationVersion />
  </Footer>

I'd say that is significantly uglier. I'd also point out that the Navigation component would still need to be documented. I don't think there's a way out of doing documentation. (Also our use of TypeScript should allow a decent editor to help with the config object where this would be impossible with composition.)

Finally, consider what would happen if the design were to change and the menu was put ahead of the footer content? How could we provide users with an upgrade path?

As a user, if I needed to give some prop to one of the menus, let's say a className or a test-id

@bertrandmc: If memory serves, at present, you can probably do this by supplying those properties on the object for the array element that pertains to your link. However, I think the real question is, "Do you have any business doing such a thing?"

It seems to me that this sort of thing is the Footer's job to sort out. the user shouldn't have to worry about it.

@bertrandmc: It would be good to get a handle on what your are (or would be) trying to achieve. It sounds like you are mostly thinking about testing. Is that right? Could you explain how data-test-id="123" would help you?

[daniel-ac-martin]

On the topic of the Footer component specifically I should point out that we don't expect users to consume it directly; rather, they would use the Page component: https://hods-poc.netlify.app/components?name=Page&show-Page-example=react

And that probably illustrates my point even better. - The children of Page should be the pages content, so the contents of the menus need to go in via props.

[bertrandmc]

Hey @daniel-ac-martin, thanks for engaging with this.

Here is an example of a real requirement we have which is not possible based on the current footer implementation. We need to add a component (which is just a string containing the build/version ID) after the navigation menu, also, new requirements will always come, it is not something we can predict.

I also agree with you about the example you gave, JSX as prop values is not ideal, but I also think large configuration objects are hard to read and to work with, the reason why we should design components that allow flexible composition via JSX as much as possible.

Finally, consider what would happen if the design were to change and the menu was put ahead of the footer content? How could we provide users with an upgrade path?

We wouldn't have this issue if the component was flexible and allowed us to compose the way we want, here is an example considering what you have mentioned before, multiple columns, adding things before and after, I build as I need.

import { Footer } from "@hods";

const { enableFeedback } = props;
const { Navigation, Column } = Footer;

<Footer>
  <SomeCustomComponentBefore />
  <Column>
      { enableFeedback && <Navigation href="/feedback" data-test-id="123">Feedback</Navigation> }
      <Navigation href="/help" className="active">Help</Navigation>
      <Navigation href="https://gov.uk/">Gov.UK home</Navigation>
  </Column>
  <Column>
    <Navigation href="https://gov.uk/about-us">About US</Navigation>
  </Column>
  <ApplicationVersion />
<Footer>

I used the footer just to illustrate the importance of this design decision, ATM you have myself, one user with one requirement: how about tomorrow? The object configuration won't scale and won't make your users happy Daniel.

[RobinKnipe]

On the topic of the Footer component specifically I should point out that we don't expect users to consume it directly; rather, they would use the Page component: https://hods-poc.netlify.app/components?name=Page&show-Page-example=react

I think we definitely should expect users to consume the Footer component (and any and every component) directly, as this would be how they can get more fine-grained control over those components. Consumers may even want to use the header/footer components outside of the currently intended scenarios.

And that probably illustrates my point even better. - The children of Page should be the pages content, so the contents of the menus need to go in via props.

I don't think that's necessarily the case, it's just the case with the current design. If components were provided through the children, you could check for a Footer child and then prefer that over the default created by the Page. Same with the Header, and Navigation, and any other common page components. There's also no reason why both methods can't be supported: props with a list of footer links for the simple case handled in the current way; a full Footer component (provided as a child of the Page) for complete flexibility and control over its contents, styles, etc. This way consumers could also specify alternative header/footer components if they so desired.

[RobinKnipe]

It seems to me that this sort of thing is the Footer's job to sort out. the user shouldn't have to worry about it.

I agree in the simplistic case; however for this toolset to allow more advanced users the flexibility they need, any such assumptions should be easy to override!

[daniel-ac-martin]

Here is an example of a real requirement we have which is not possible based on the current footer implementation. We need to add a component (which is just a string containing the build/version ID) after the navigation menu, also, new requirements will always come, it is not something we can predict.

@bertrandmc: Okay, but that's not a part of the component as it has been designed. (Although this is the first time the Footer component has been properly codified so there are likely to be some short comings!) Do you have any screenshots to show what you mean? Do you just want to have a version number on the bottom right that is not a link?

If your use case is legitimate, it seems to me that this should be supported as a feature of the component. i.e. A version prop.

It should be noted though that the GDS Footer component does not explicitly support a version number. So I wonder if this is something that GDS have already rejected. Most likely they would argue that a version number serves no benefit to the user.

If you can convince the design community of the need to support this, we will certainly support this use case.

I expect the question will be, "Why do you want to include this version number on the page?"

Basically I think this is a design question rather than a development question. (@karypun: Any thoughts?)

new requirements will always come, it is not something we can predict.

Yes, but you are either using the HO design or you are not. The purpose of these components is to help projects use the design. They should not be so loosely defined as to allow projects to break out of the design. If you want to not use the design in a particular way then it makes sense for you to implement your own component for that. (But as I state above, perhaps you have a legitimate use case that should be supported.)

Alternatively, if we decide to support a very loose interface (and therefore make fewer design decisions centrally) we could provide two components, one very simple where you bring your own menu, and one well defined. (I doubt we should lose the well-defined one though.)

But as I say, this is a design question. You are effectively saying, "This design doesn't meet my projects needs", not merely the implementation.

[daniel-ac-martin]

I think we definitely should expect users to consume the Footer component (and any and every component) directly, as this would be how they can get more fine-grained control over those components.

@RobinKnipe: Why? After all, you can provide those same props in the Page component. If we can safely abstract Footer away, we should.

As far as I can see, the Footer has no use outside the bottom of a page. So why not simply provide a Page component that does that?

Consumers may even want to use the header/footer components outside of the currently intended scenarios.

Well the component will still be there for them to use.

you could check for a Footer child and then prefer that over the default created by the Page

I don't think you could do this. Not reliably anyway. React doesn't really provide a way to inspect the contents of the children.

I agree in the simplistic case; however for this toolset to allow more advanced users the flexibility they need, any such assumptions should be easy to override!

No-one is forced to use every component. If a project wants to reject the design decisions behind a component they can create their own component and take on the maintenance burden of that.

The nice thing about (well written) components is that you can mix and match them.

The trade off that is being discussed here is roughly as follows:

A well defined interface	A loose interface
Easy to consume	Harder to consume
More limited in function	Less limited in function

I don't think it is right to embrace an interface that is so loose that it allows projects to break out of the design. The interface should be as constrained as the design.

I don't want to be responding to bug reports with "You're using the components wrong." - The interface should make it impossible to misuse the components (at least in TypeScript).

[daniel-ac-martin]

@bertrandmc: Another way to look at this would be to say, that if your use case is legitimate then we should probably provide and maintain a central solution for it. That way, future projects can also benefit. (Rather than providing a very loose interface and off-loading the problem into projects like yours.)

[RobinKnipe]

@daniel-ac-martin

The purpose of these components is to help projects use the design. They should not be so loosely defined as to allow projects to break out of the design.

Don't forget that part of the intention for this toolset is to help provide for internal uses; it's likely that such cases shouldn't be constrained by expectations of GOV.UK

[daniel-ac-martin]

@RobinKnipe: This is the Home Office specific one. It implements the design of the Home Office Design System as managed by the DesignOps team. i.e. If there's a problem with the design, we have a team that can address that.

[RobinKnipe]

@daniel-ac-martin - It's perfectly possible (and trivial) to inspect React children:

React.Children.map(this.props.children, (child) => {
  if (child.type === Footer) { ... }
});

More here: https://mxstbr.blog/2017/02/react-children-deepdive/

[RobinKnipe]

@RobinKnipe: This is the Home Office specific one. It implements the design of the Home Office Design System as managed by the DesignOps team. i.e. If there's a problem with the design, we have a team that can address that.

Then I'd recommend renaming the component so that is absolutely clear. Perhaps there should be a separate package (or other appropriate grouping) for components that are tightly constrained to the HODS?

[daniel-ac-martin]

All of the components in this repo are HODS components.

Whilst we shouldn't do unfriendly things like hard-code the font (which would make them unnecessarily hard to use in other contexts) we also shouldn't bend other backwards to support use cases outside of the Home Office, especially not at this early stage.

But I don't think we are talking about such a case here. Which is why this is a question for our designers.

[RobinKnipe]

All of the components in this repo are HODS components.

Oh yeh! Good point. I thought this was the other repo!

[karypun]

I expect the question will be, "Why do you want to include this version number on the page?"

Basically I think this is a design question rather than a development question. (@karypun: Any thoughts?)

@bertrandmc @daniel-ac-martin I'd like to understand a bit more about what the actual design problem is. Regarding the version number needed on the page, who does this intend to serve, what are the needs for this and could there be alternatives for providing it besides building it into the footer? Just wondering whether this applies to specific instances or becomes a blanket-wide feature for all.

New requirements will always come along. Is it more a question of what might a baseline template or structure could look like which could then scale to accommodate any new features or requirements?

[RobinKnipe]

If the version number is solely for Devs, it could be included in a more subtle way, i.e. somewhere in the <head> element. If there were an expectation for the user to provide it, shouldn't that be automatically handled by the feedback mechanism?

[karypun]

@RobinKnipe I'm not sure what you mean by feedback mechanism but if there are alternative options depending on who the intended audience is regarding this particular instance with the versioning, then perhaps it's worth exploring them.

[RobinKnipe]

@karypun :wave: I mean these kinds of things...

Choosing those actions should capture any relevant information from the page (URL, version, etc.)

[bertrandmc]

@karypun The version number was just to illustrate that the lack of flexibility with the components will be problematic, the design is strange to React users because it goes against the React principles of composition and common abstraction.

But by the conversation it seems to me this choice is because the team thinks locking up the components will be better, which I disagree but respect, but as a user, unfortunately It will be hard to depend on a library that can potentially become a bottleneck for our projects, our teams need to be able to implement the requirements as they come, instead of depending on external PRs or feature requests, the design you are adopting is the exactly reason why we are moving away from the library we use ATM.

[daniel-ac-martin]

Hi @bertrandmc,

Thanks for that; I think I understand a bit better now.

I'd like to point out that I'm not entirely opposed to composition, I simply think that it should be used with care. I also disagree with the API provided by govuk-react-jsx. My understanding is that it tries to replicate the Nunjucks API provided by GDS. I think that is a mistake; what is appropriate in Nunjucks is not necessarily ideal in React. (I think I might have discussed this with one of your colleagues.)

My position is that we should allow for composition where is makes sense. For example, the HODS Footer does allow for some composition. Take a look at the last example on: https://hods-poc.netlify.app/components?name=Footer&show-Custom-example=react

The same is true for the GovUK-style one here: https://not-govuk.netlify.app/components?name=Footer&show-Nav%20and%20meta-example=react

The Alert component also supports composition: https://hods-poc.netlify.app/components?name=Alert&show-Error-example=react&show-Success-example=react

In fact, I suspect you could achieve what you wanted above as long as you provided your own navigation menu.

My only objection is to providing loosely defined interfaces without good reason. (Hence the standard navigation being defined by a well-typed array.)

Of course, the interfaces we're defining will not be perfect first time. We should change them in response to feedback when there is a good reason to do so. But I think it is better to start off with a strict interface and loosen it as required; it's very hard to tighten up a loose interface later on without upsetting people. (And tightening up a loose interface is always a breaking change.)

We welcome discussion on the interfaces we are providing so I hope that you will continue to provide feedback as we add more components to library.

[penx]

I just found this project. Nice work, some good dev/tech patterns in here. I think I should probably be using publishConfig in govuk-react :-)

Incidentally, overuse of composition was one of the reasons that I grew to dislike the govuk-react library.

... but sorry to hear that 😄

I'm in the process of clearing out the govuk-react backlog so if there's any other feedback please do let us know by raising issues. I'd also be keen to know if anyone wants to help maintain it.

I would say that one of my regrets with govuk-react was not having a consistent approach to composition. This is something I hope to address.

For example, I describe a consistent approach to form field composition here (recently edited): https://github.com/govuk-react/govuk-react/issues/164

I completely understand the usefulness of having a list component that accepts an array, but what if someone wanted to later add an onClick to the third item after you had already built and distributed your non composable component? This then means you need to extend your array API and release every time you want to add a basic feature like this, and is something that could be handled by JSX through composition.

My personal preference these days is to make everything composable/atoms and then have a single utility component that uses/wraps these atoms, so users can choose between the composition approach or the single component approach dependent on what suits them. The 2 caveats are this should be done consistently and if people compose components in an unexpected/unsupported way then things could break for them via updates, so you need to document the intended uses.

[penx]

This POC I wrote my also be of interest:

https://github.com/penx/govuk-frontend-react

In particular the convention on moving from existing BEM architecture to composable React components:

https://github.com/penx/govuk-frontend-react#bem

Also worth noting is that when you use component composition, if you attach the sub component as a property to the main component, this breaks tree shaking. You can get around this by exporting each component on its own and doing:

import * as List from '@my-ds/list'

<List.Root>
  <List.Item />
</List.Root>

[penx]

I'd also consider, in case you decide to use composition later, how you build your API so that this doesn't introduce breaking changes.

e.g.

Initial non composed API on the default export

import MyComponent from './my-component'

// One component to rule them all
const Thing = () => <MyComponent items={['a', 'b', 'c']} onClicks={[null, thing, null]} />

Opt-in composed option added later on named exports

import * as MyComponent from './my-component'

// A composable option
const Thing = () => <MyComponent.Root>
  <MyComponent.SubNode>a</MyComponent.SubNode>
  <MyComponent.SubNode onClick={thing}>b</MyComponent.SubNode>
  <MyComponent.SubNode>c</MyComponent.SubNode>
</MyComponent.Root>

[penx]

@RobinKnipe @daniel-ac-martin are you still using govuk-react? I recently released 0.10.0 with published types. Keen for any feedback, or if you want to discuss anything over a call sometime.

[daniel-ac-martin]

@penx: Sorry, I've been meaning to sit down and write a proper reply for ages.

We do still have a repo that is using govuk-react at https://github.com/UKHomeOffice/lev-react-components . It's a bit unloved though. (@aarongill1 will know more about it than me.)

Thanks for the offer of a call. It would be nice if a few of us started discussing ideas around best practices with React and also these component libraries and how to maintain them.

At the moment we've got some duplication of effort. Which looks quite bad on the surface, but is ultimately the result of us having different opinions on the best way to go about this stuff (and slightly different goals!). If we start talking some more we might come to common understanding that would allow us to concentrate our efforts.

In the meantime maybe we could have a slack channel on X-Gov or some other text-based communication?

[penx]

At the moment we've got some duplication of effort. Which looks quite bad on the surface, but is ultimately the result of us having different opinions on the best way to go about this stuff (and slightly different goals!). If we start talking some more we might come to common understanding that would allow us to concentrate our efforts.

@daniel-ac-martin Your NotGovUK library looks good, and has some patterns in it that I was planning to be in govuk-react at some point. But we haven't had too many people get in touch with us about usage, issues, architectural suggestions etc. - or offers of contributions - so it's hard to prioritise. I think we may find we're more aligned than you think, it's just that govuk-react isn't quite where we want it to be yet.

In the meantime maybe we could have a slack channel on X-Gov or some other text-based communication?

Unfortunately I'm not on the X-Gov slack anymore, but yes lets find a way to DM each other first and then figure out a slack channel or similar for wider discussion. I'm on Twitter at https://twitter.com/penx and you should be able to find me on LinkedIn (I think I sent you a request yesterday, assuming I found the right person!)