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 transform from core/list block #14

Merged
merged 7 commits into from
Feb 8, 2022
Merged

Conversation

cr0ybot
Copy link
Contributor

@cr0ybot cr0ybot commented Feb 5, 2022

This PR addresses the feature request in #11 .

The transform keeps the list block's type (ordered, unordered):

list2superlist

It also attempts to pass other attributes from the list block to each resulting paragraph, which allows for some things like Typography settings to come over. Unfortunately, the font family setting is currently still experimental and exists on the list block but not the paragraph block (???), but once it is supported it should come over.

list2superlist-attributes

I'm happy to try to attempt a similar transform from the columns block, maybe in a separate PR?

@aurooba
Copy link
Member

aurooba commented Feb 5, 2022

Whoa! I'm mind blown that someone wants to contribute to Super List. Thanks @cr0ybot! Columns transformation should definitely be a separate PR, easier testing. I'll review this later this weekend, but a quick perusal of the code and your super helpful GIFs looks great!

@aurooba
Copy link
Member

aurooba commented Feb 5, 2022

As of #22, both the Super List block and Super List Item block have full typography support (all experimental properties are also supported, since they are supported in core). In light of that, should this transform grab typography attributes and attach it to the paragraph or to the Super List Item itself? I'm leaning towards Super List Item.

@cr0ybot
Copy link
Contributor Author

cr0ybot commented Feb 5, 2022

Tricky. If the main Super List block supports these now then maybe it makes sense to apply the attributes to the main block? It would be a lot less to edit if the user didn't want the typography settings on every single item. Happy to update the PR with whichever you think is best.

@aurooba
Copy link
Member

aurooba commented Feb 5, 2022

If the main Super List block supports these now then maybe it makes sense to apply the attributes to the main block?

Right, because there's typography settings on the the entire core/list (even now it blows my mind that individual list items in core/list aren't manipulable!). You're totally right, it should just apply to the main block. Let's go with that, it's the best option. Would love for you to update the PR!

I'm done working on this plugin for this weekend so there won't be a zillion other changes till Tuesday at the earliest. 😅

@aurooba aurooba linked an issue Feb 5, 2022 that may be closed by this pull request
# Conflicts:
#	build/superlist.asset.php
#	build/superlist.js.map
…perlist block (instead of to each new paragraph)
@cr0ybot
Copy link
Contributor Author

cr0ybot commented Feb 5, 2022

PR updated.

This strategy works really well! I was delighted to see the typography settings applied on the main superlist block affect the bullets/numbers, and since you're supporting font family that works too!

Note that I'm just blindly copying over all of the other original list attributes to the main superlist block, so any other supports that superlist shares with the core/list block should automatically transfer. Also note that attributes like "start" (numbered list first number), "reversed", and "placeholder" will come over too, if you support those attributes in the future.

list2superlist-typography

@aurooba
Copy link
Member

aurooba commented Feb 6, 2022

@cr0ybot I just finished testing out the transform! It works beautifully! The only thing it doesn't handle is sub list items in core/list: they become top-level super list items in the transform. So there's two options here:

  1. We can merge this and I can work on an additional PR so any lists inside a core list item become a nested super list, or
  2. You can update this PR to include that feature as well.

Let me know which way you'd like to go and we'll take it from there. This is awesome, thank you so much for your work on this, I super duper love it.

@cr0ybot
Copy link
Contributor Author

cr0ybot commented Feb 6, 2022

Oops, I'm embarrassed I didn't even think about nested lists. I've got a little time right now, I should be able to sort that out in this PR.

@aurooba
Copy link
Member

aurooba commented Feb 6, 2022

Don't be embarrassed! Fresh eyes help. :)

@cr0ybot
Copy link
Contributor Author

cr0ybot commented Feb 7, 2022

Dealing with nested items is turning out to be more complicated than I expected, but I've got some ideas. I can't rely on the @wordpress/rich-text APIs solely, particularly split, because it drops information about nesting. I'm taking a DOMParser approach, will update soon if it works out.

@cr0ybot
Copy link
Contributor Author

cr0ybot commented Feb 8, 2022

Phew, this was a good brain workout!

PR updated with a completely new list parser that handles nested lists and outputs nested superlists, keeping all of the text formatting I could throw at it!

Screen Recording 2022-02-07 at 8 34 58 PM

There are lots of comments in the code, so hopefully it isn't too hard to sort out what is going on, but the gist is that I'm using the native DOM parsing mechanism to read the original list block's contents and recursively traverse it to output the correct blocks in the correctly nested order.

This transformation method will definitely not work if the core/list block is ever updated to include nested blocks, but doing that transform should be much simpler than this!

@aurooba
Copy link
Member

aurooba commented Feb 8, 2022

Flawless! Super clean code and putting my lack of documentation in the plugin to total shame. I will fix that pronto. 😅

@aurooba aurooba merged commit e1ef7fd into createwithrani:main Feb 8, 2022
@aurooba
Copy link
Member

aurooba commented Feb 8, 2022

For the record, @cr0ybot, the PR to for innerBlocks support for core/list hasn't been updated since November of last year, and I don't really think it's a priority for the team. WordPress/gutenberg#35870

I might do it, but right now I have a couple other projects in the Gutenberg project I want to handle first.

@cr0ybot
Copy link
Contributor Author

cr0ybot commented Nov 2, 2022

@aurooba Looks like nested blocks were finally implemented with WordPress/gutenberg#39487. Probably need to revisit this code to check for the new block version. The upside is that it would be a heck of a lot easier to transform nested blocks than the previous list block implementation!

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.

Allow for transformation from the core/list block
2 participants