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

RFC 94: Union Block #94

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

RFC 94: Union Block #94

wants to merge 11 commits into from

Conversation

jams2
Copy link

@jams2 jams2 commented Mar 11, 2024

@jams2
Copy link
Author

jams2 commented Mar 11, 2024

Note: supersedes #66

@Scotchester
Copy link
Contributor

This is a really great RFC! I agree that it would be a very valuable addition to Wagtail, and you've very clearly presented why, and thoroughly explained how you would do it. Well done!

One question to confirm my understanding/assumptions: For the link block example, in order to also set the text of the link, you would presumably suggest this sort of structure?

class LinkBlock(StructBlock):
    text = CharBlock()
    link = UnionBlock([("page", PageChooserBlock()), ("url", URLBlock())])

(or link = MyLinkUnionBlock(), which is defined in its own class above)

I might suggest extending the examples presented to cover that complete link block replacement scenario.

The only bit that I'm a little wary of is introducing this new UI paradigm of using a ChoiceField to choose what you want the sub-block to be. As you point out in referring to wagtail-link-block, this is something that has been done in a number of packages (or custom implementations), but we may want to discuss this UX with @benenright or other design stakeholders before adopting it in Wagtail core.

@jams2
Copy link
Author

jams2 commented Mar 11, 2024

Thanks for the feedback @Scotchester !

Yes, the example you point out is incomplete (and your assumption is correct) - this is an oversight and I will update it.

I will be very happy to receive guidance/feedback on the particulars of the UI/UX.

@Nigel2392
Copy link

Nigel2392 commented Mar 16, 2024

This is a really good RFC. Love it!

Could this possibly be extended to make it more dynamic? Why limit ourselves to just a single type.

Maybe implement some sort of base class for the main logic (most of this is already in the streamblock); subclass that to filter out the item(s) on the javascript and use the unionblock class to achieve the same on the python side. This would then still allow a list of allowed types based on some arbitrary filter method instead of just a single type. It could be a list of one [type].

In this case - the "chooser" (radio, select, whatever); would "tell" the javascript adapter which type is allowed; the js definition (the subclass I mentioned) would then filter out to only allow that block type.

This I think would also keep work minimal; as most of the logic for this would then already be implemented in the streamfield/streamblock.

Keeps it extendable for developers 😉

As for the choicefield; my input is likely not really important - that's the UX guy's job, but I say no.
Something which aims to implement more use cases for a block should not then limit itself by the amount of blocktypes to be chosen.
What if you have 10 choices? I would not want 10 different select boxes.

Relevant slack discussion for extra info

Me

It looks like it wouldnt be too hard to extend this to dynamically show/hide menu items in the streamfield menu based on for example; meta attributes (or stimulus controllers??? 😉 )
https://github.com/wagtail/wagtail/blob/cd50955b4984b156250d1382b2cc9c880779b57c/client/src/components/StreamField/blocks/StreamBlock.js#L119
I've never used react; but this seems like it's pretty doable.
I remember questions for this being asked on platforms like stackoverflow before - is there any particular reason this hasn't been implented? (edited)

LB

4 hours ago
Hey Nigel. Are you talking about dynamically hiding block addition options?
What's your main use case?
As for why a feature hasn't been implemented, that's just the nature of software and open source. It's not been a priority or no contributions to get the feature in.
Nonetheless, if you have a solid idea and feel up to raising an issue to propose it - go for it. There may also already be an issue so do a quick search.
You may also be interested in reading the Union Block RFC that's just gone up.
#94

Me

< 1 minute ago
I though up an example; I'm currently wanting to do something similar for a menu system.
Say you have 5 different card struct blocks - they would often share the same container.
To keep things uniform you would then need to define 5 different classes of those wrapper blocks - you would not want 2 different types of card next to eachother. This takes up a lot of space; and it makes everything less intuitive inside of the addition menu.
Instead of that; we could possibly implement a method on the structblock definition (js) to do an arbirtrary check.
In this case; it would check for the type of the first card chosen, and then only allow choices of that type.
I realize I am describing the union block; but I think this can be extended. Thanks for the link; now I know where to take this idea 😉
I was more so asking for a reason because the question might already have been asked and answered - maybe it was thought of as a bad idea 😛

@jams2 jams2 changed the title RFC : Union Block RFC #94: Union Block Mar 16, 2024
@jams2 jams2 changed the title RFC #94: Union Block RFC 94: Union Block Mar 16, 2024
@jams2
Copy link
Author

jams2 commented Mar 16, 2024

Hi @Nigel2392, thanks for the feedback. The functionality I'm proposing would allow for your use case, if I've understood it correctly. You would be able to define a block like this:

class CardGroupChooser(UnionBlock):
    card_option_1 = ListBlock(Card1())
    card_option_2 = ListBlock(Card2())
    card_option_3 = ...

For each instance of CardGroupChooser in your stream field, editors would be able to choose one of the ListBlock types.

Something which aims to implement more use cases for a block should not then limit itself by the amount of blocktypes to be chosen.
What if you have 10 choices? I would not want 10 different select boxes.

I have proposed making the chooser's widget customisable, so that developers are able to select a compatible widget other than the default (e.g. a Select widget).

@Nigel2392
Copy link

Nigel2392 commented Mar 16, 2024

Hi @Nigel2392, thanks for the feedback. The functionality I'm proposing would allow for your use case, if I've understood it correctly. You would be able to define a block like this:

class CardGroupChooser(UnionBlock):
    card_option_1 = ListBlock(Card1())
    card_option_2 = ListBlock(Card2())
    card_option_3 = ...

For each instance of CardGroupChooser in your stream field, editors would be able to choose one of the ListBlock types.

Something which aims to implement more use cases for a block should not then limit itself by the amount of blocktypes to be chosen.
What if you have 10 choices? I would not want 10 different select boxes.

I have proposed making the chooser's widget customisable, so that developers are able to select a compatible widget other than the default (e.g. a Select widget).

I was more thinking of something like this (rough sketch, heavily simplified, implementation would vary by a lot)

class CardGroupChooser(UnionBlock):
    # example for the __union_type__ input; just to make choices clear
    option = blocks.ChoiceBlock(
        choices=[
            ("happy", _("Happy")),
            ("sad", _("Sad")),
        ],
    )

    card_options = StreamBlock([
        ("card_option_1", Card1(condition="happy"])),
        ("card_option_2", Card2(condition="happy"])),
        ("card_option_2", Card2(condition="sad"])),
    ])

This would take away from it being a true union-like block; but would in turn give developers lots more options.

This could even be taken one step further:

class CardGroupChooser(UnionBlock):
    option = blocks.ChoiceBlock(
        choices=[
            ("happy", _("Happy")),
            ("sad", _("Sad")),
        ],
    )

    weather = blocks.ChoiceBlock(
        choices=[
            ("sunny", _("Sunny")),
            ("rainy", _("Rainy")),
        ],
    )

    card_options = StreamBlock([

        # Only show if we are happy
        ("card_option_1", Card1(conditions=["option.happy"])),

        # Only show if the weather is rainy and we are happy
        ("card_option_2", Card2(conditions=["option.happy", "weather.rainy"])),

        # Only show if we are sad
        ("card_option_3", Card3(conditions=["option.sad"])),
    ])

This would mean we could still get 2 different block types if the stars align.
But! you would be able to easily subclass this to create your own union block.

Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

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

Solid proposal @jams2! Reads very well. I’d like to see more demonstration of how much this helps with the UX and the code. It’s clear to me there’s room for improvement on both fronts but not convinced the ROI is high enough with the proposal as-is?

I’d also like to see alternatives considered – how much thought have you given to other options? I have two in mind that seem to me like they’d have higher return on investment for UX and code improvements, but potentially score lower on API semantics:

  1. Implement similar generic improvements to what this suggests but in a backwards-compatible way.
    • Tweak the StreamField UI for those "list of one item at most" use cases, perhaps add a way to define a default in StreamBlock.
    • Introduce better support for conditional fields across Wagtail, perhaps utility methods to reduce the boilerplate.
  2. Work on Single interface for choosing links to anything #381 as a separate data type

There are two questions I’d also like to have an answer for:

  1. Are there a lot of other widespread enough use cases for this beyond links of different types?
  2. If this is primarily for links, is StreamField really the right approach or should we be looking at other options?

Comment on lines +55 to +61
![Link block implemented stream block style](./assets/094/stream-style-link.png)

The UI generated for inserting a link requires editors to first select the "+" button to insert a block, and then choose the block type. In the typical case that the link value is _required_ this creates dissonance between what is required by data validation and what is communicated to users by visual language - requiring users to insert a block when that block is required is a sub-par experience. Compare this to a link block implemented as a `UnionBlock`:

![Link block implemented union block style](./assets/094/union-style-link.png)

In this example all fields required to make a valid submission are immediately present in the UI, which clearly communicates the requirements of the system to users and prevents a class of validation errors from occurring.
Copy link
Member

Choose a reason for hiding this comment

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

The practical UX differences as I understand them:

  1. The StreamField block chooser is instead a radio group (or could be a <select> I suppose)
  2. There’s a default link type selected, so we display the fields for that link type from the get-go
  3. There’s no disabled "+" button to add any more links of that type

If that’s it, I’m not clear why this couldn’t be done within existing StreamField capabilities?

And I’m also not convinced that the proposed UI is any clearer, because:

  1. Now selecting which set of fields is available via the "Link type" looks the same as any other field of a block.
  2. If the link is optional, the user would now have to remove the auto-added default block.

Copy link
Author

Choose a reason for hiding this comment

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

The UI pattern that I want to get away from is having the options hidden away behind the "+" icon and needing that extra click to discover the options, when it could be implemented such that all of the information is available in the UI. The idea is that the UnionBlock would be used for a more focused set of options than StreamBlock, which can often become a "kitchen sink" of unrelated block types, and have a quantity of options such that displaying them persistently in the UI is unpractical.

I do feel that this proposed solution goes against the grain of the current blocks UI. When I wrote this, I had in mind the idea that users shouldn't have to interact with the page to reveal the information they need to accomplish their task.

class LinkBlock(blocks.StructBlock):
title = blocks.CharBlock()
link = LinkChooserBlock()
```
Copy link
Member

Choose a reason for hiding this comment

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

I think it’d be nice if the examples really were equivalent. As-is I’m not clear if LinkStructValue would look the same with UnionBlock, or would be radically simpler.

Copy link
Author

Choose a reason for hiding this comment

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

The intention here is that a developer-defined LinkStructValue wouldn't be necessary for common use cases of UnionBlock. There is no longer a need to select an element of a sequence, and type specific logic could be pushed onto the constituent block types (e.g. in their get_context method, or their individual templates).

link = LinkChooserBlock()
```

This approach to the `LinkBlock` problem requires developers to write less code - significantly less when taking into account the custom JavaScript required when taking the approach illustrated by `wagtail-link-block`.
Copy link
Member

Choose a reason for hiding this comment

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

wagtail-link-block displays multiple fields from its StructBlock for specific link types, so I’m not convinced this is a fair comparison. If I understand this correctly, switching from StructBlock to UnionBlock would require duplicating those fields within multiple StructBlock as options in the union?

Copy link
Author

Choose a reason for hiding this comment

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

The sentence here is a little confusing, as "code that the developer of a particular Wagtail CMS instance needs to write" appears to be conflated with code that is part of a library.

That being said, fields shared by multiple members in a union could be implemented using StructBlock inheritance, e.g.:

class BaseFooBlock(StructBlock):
    a = SomeBlock()
    b = SomeOtherBlock()


class FooBlock(BaseFooBlock):
    # ... extra fields


class BarBlock(BaseFooBlock):
    # ... extra fields


class FooBarUnion(UnionBlock):
    foo = FooBlock()
    bar = BarBlock()

In this example, both foo and bar would share the a and b fields without duplicating their declaration.


`UnionBlock` is a new block type that allows editors to select a block type from a set of types defined by the developer, and then insert a single value for that chosen type.

For each instance of `UnionBlock`, Editors should be presented with a `ChoiceField`, with one option for each sub-block that is a member of the union. When they make a selection, the UI should be updated so that the native form widget for the selected block type is presented. Only a single form field for the block's value should ever be presented. A default value must always be provided, as an empty choice requires editors to make an interaction to reveal a form field for the value, when they have already made an interaction indicating that they wish to enter a value when they selected the `UnionBlock` (or the block containing it) in a `StreamBlock`.
Copy link
Member

Choose a reason for hiding this comment

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

I don’t understand how this would work for optional links 🤔. For example here:

class CTABlock(blocks.StructBlock):
    text = blocks.CharBlock()
    link = LinkChooserBlock(required=False)

If LinkChooserBlock was a UnionBlock, it’d need an explicit default of "EmptyBlock"?

Copy link
Author

Choose a reason for hiding this comment

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

I think this is an oversight. We would have to allow for optional members, so an empty choice would need to be available — it could have a label like "Please select a link type". Perhaps this should be tweaked to say something along the lines of "If the UnionBlock is a required sub-block, a default value must always be provided..." — my initial line of thought was to minimise the number of interactions required by the editor to enter a value.

I think we could use null to represent "no choice" in an optional UnionBlock at the JSON level.

@kaedroho kaedroho mentioned this pull request Apr 4, 2024

The serialised value of a `UnionBlock` should be a JSON object, with the following required keys:

- `type` - the name of the `UnionBlock`;
Copy link
Contributor

@kaedroho kaedroho Apr 4, 2024

Choose a reason for hiding this comment

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

One feature of the block described in RFC 66 was compatibility with the StreamBlock JSON representation. So the following types would have the same representation in the JSON:

StreamBlock({
    "paragraph": ParagraphBlock(),
    "image": ImageChooserBlock(),
})

ListBlock(EnumBlock({
    "paragraph": ParagraphBlock(),
    "image": ImageChooserBlock(),
})

I thought this would allow us to refactor StreamBlock to be a special case of ListBlock so all sequences in streamfields could be handled the same way, and potentially make features like migrations commments, etc easier to implement.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kaedroho This probably isn't feasible, now that ListBlock itself has a StreamBlock-like JSON representation where the items are blocks (dicts with an id attribute and type="item"), rather than plain values. To make the representations match, ListBlock would need to include a special case for when its child block is a UnionBlock and drop the extra wrapper.

@jams2
Copy link
Author

jams2 commented May 19, 2024

Thanks for the feedback @thibaudcolas! I'll address a couple of points from your parent post here.

Implement similar generic improvements to what this suggests but in a backwards-compatible way.

Work on wagtail/wagtail#381 as a separate data type

I feel that this would be missing an opportunity to introduce a new, useful, fundamental building block that addresses a common rough corner in user code. I do think the core value here is provided by having unions present in the "stream field type system", so that developers don't have to shoe-horn data that is best modeled as a union into a sequence type.

I do agree that having a "link type" in Wagtail could be beneficial, but even if that were implemented, I would still advocate for the introduction of UnionBlock.

Are there a lot of other widespread enough use cases for this beyond links of different types?

Speaking from my own experience, I've seen other uses of StreamBlock that could benefit from being treated as UnionBlocks, e.g. lists of cards where there are multiple "types of cards" available to populate the list, or a choice from multiple types of "callout" blocks. However, I think that while links are a useful demonstration of the use case, the real value is in fleshing out the type system so that data can be modeled more accurately in stream fields.

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

Successfully merging this pull request may close these issues.

6 participants