Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add generic component block #4527

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add generic component block #4527

wants to merge 1 commit into from

Conversation

KludgeKML
Copy link
Contributor

@KludgeKML KludgeKML commented Dec 12, 2024

  • Lots of our blocks are wrappers around components, so adding a general component block allows us to use a wider variety of components without adding a lot more blocks (and potentially we could clean up existing blocks, but that should wait a little).
  • The allowlist of components is intended to include all components that are not: Layouts, Banners, Form Elements, Page Furniture that does not work out of a specific place, or components marked as in an Experimental state.
  • Adding this unlocks some possibilities for replacing history pages with landing_page document types, since it allows us to start using the Page title component used in those pages.

- Lots of our blocks are wrappers around components, so
  adding a general component block allows us to use a wider
  variety of components without adding a lot more blocks
  (and potentially we could clean up existing blocks, but that
  should wait a little).
- The allowlist of components is intended to include all
  components that are not: Layouts, Banners, Form Elements,
  Page Furniture that does not work out of a specific place,
  or components marked as in an Experimental state.
@@ -0,0 +1,45 @@
module LandingPage::Block
class Component < Base
ALLOWED_COMPONENTS = %w[
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd start very small with this list and expand it later if needed, rather than start big and then whittle down. There's a lot of components in this list that I'd have questions around how/if they'd be used on these pages.

For a starting list I'd suggest only action_link, big_number, cards, document_list, govspeak, heading, lead_paragraph, share_links - basically the ones we've implemented already.

@@ -61,6 +62,17 @@ A wrapper around the [Big number component](https://components.publishing.servic
label: amount of money that looks big
```

#### Component

Renders whatever component from the publishing components gem is specified with the `component_name` value. All attributes _except_ `component_name` and `type` are passed directly to the component. Note that `component_name` should be the component in snake case (ie as it appears at the end of the url in the component guide). The example here will render identically to the Big Number example directly above, for instance.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: no need to emphasise except.

Copy link
Contributor

@leenagupte leenagupte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea in principle, but wonder if it will come undone if we find that we need to be able to add different styling depending on how the component is used. Or if it needs a specific theme applied, so we need to create a block for it, but now, as with big number, we have two different ways of calling the component.

I'm also concerned about how a CMS will "know" which components are allowed. Will the list of allowed components have to be maintained in two places?

end
let(:subject) { described_class.new(blocks_hash, build(:landing_page)) }

describe "allowed list of components" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be describe "#component_name" then have the following two nested contexts/tests:

  • context "when the component_name is in the allowed list"
    • it "sets the components name"
  • context "when the component_name is not in the allowed list"
    • it "raises an error"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants