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

feat!: Support alternate ElementWrapper #320

Merged
merged 19 commits into from
Nov 30, 2023
Merged

Conversation

rhystmills
Copy link
Contributor

@rhystmills rhystmills commented Nov 20, 2023

What does this change?

This adds support for a using an alternate ElementWrapper for elements. In this case I'm exporting a new AltStyleElementWrapper from prosemirror-elements, making use of components from the existing ElementWrapper with some design changes. The new layout aims to feel more spacious within - for example when an element makes up most of a piece of content.

The controls from ElementWrapper are now moved to their own file (WrapperControls) so that it's simpler to compose a new ElementWrapper.

This PR includes a breaking change to createReactElementSpec, which will now take an options object as its argument rather than separate ordered arguments. The footprint for this PR looks fairly large because I've needed to update the demo element forms to provide arguments in this format.

We are aiming to use these styles in the upcoming 'key takeaways' element, which will represent the majority of content in a key takeaways article.

Since Composer now provides the Form components itself, we will need to provide new or altered components there to restyle the form itself. I've updated the styles used in the demo components here to mirror the intended experience in Compose.

This PR also makes some changes to the overall demo styles to make it slightly more user-friendly.

In the demo app here:

image

In Composer:

image

How to test

Run the demo locally and click 'Add Alt Style' alongside another element. Do they have different styles? Are there any React-related errors?

@rhystmills rhystmills requested a review from a team as a code owner November 20, 2023 14:21
@rhystmills
Copy link
Contributor Author

rhystmills commented Nov 20, 2023

This can be used locally with flexible-content via yalc with this Composer branch: https://github.com/guardian/flexible-content/pull/4449

To do so:

  1. Publish this branch locally with yarn yalc
  2. Run yalc add @guardian/prosemirror-elements in the composer subfolder of flexible content locally, then run npm install.
  3. Run ./dev-start.sh in flexible-content, and activate the key takeaways feature switch (see the menu with shift-fn-f12 and refresh)
  4. Does the alternately styled key takeaways element appear in the element insertion submenu?

@rhystmills
Copy link
Contributor Author

Had a chat with @jonathonherbert off GitHub - have a more flexible API change that could achieve these changes - enabling us to replace the ElementWrapper itself, rather than specifying styles, and controlling the form appearance from the elements passed in there, rather than by threading a style argument through.

@rhystmills
Copy link
Contributor Author

That's now implemented!

@rhystmills rhystmills changed the title feat!: Support alternate element styles feat!: Support alternate ElementWrapper Nov 27, 2023
Copy link
Contributor

@simonbyford simonbyford left a comment

Choose a reason for hiding this comment

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

This looks excellent, nice work!

I wonder about the name AltStyleElement but can't think of a better one 🤔 I guess we're avoiding calling it anything to do with Key Takeaways as we're hoping to use it more broadly?

/>
</FieldLayoutVertical>
),
elementWrapper: AltStyleElementWrapper,
Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect 👌

Copy link
Contributor

@jonathonherbert jonathonherbert left a comment

Choose a reason for hiding this comment

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

This looks really neat. One suggestion about naming and commenting.

@rhystmills rhystmills merged commit 0e53c09 into main Nov 30, 2023
3 checks passed
@rhystmills rhystmills deleted the rm/element-native-styles branch November 30, 2023 10:16
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.

None yet

3 participants