Skip to content

Better ActionText plain text output for nested lists#37976

Merged
dhh merged 7 commits intorails:mainfrom
swanson:action-text/plaintext-lists
Oct 12, 2021
Merged

Better ActionText plain text output for nested lists#37976
dhh merged 7 commits intorails:mainfrom
swanson:action-text/plaintext-lists

Conversation

@swanson
Copy link
Contributor

@swanson swanson commented Dec 15, 2019

Summary

When using to_plain_text on action text content, nested lists are not rendering in an expected way. Out of the box, the rich text editor allows arbitrary nesting of lists using the "Increase" and "Decrease Level" buttons.

Basic case with one nested item:

ActionText::Content.new("<ul><li>Item 1<ul><li>Item a</li></ul></li></ul>").to_plain_text

=> "• Item 1• Item a"

Expected:

• Item 1
  • Item a

Complex case with multi-level nested and mixed list types:

ActionText::Content.new("<ul><li>Item 0</li><li>Item 1<ul><li>Item A<ol><li>Item i</li><li>Item ii</li></ol></li><li>Item B<ul><li>Item i</li></ul></li></ul></li><li>Item 2</li></ul>").to_plain_text

=> "• Item 0
• Item 1• Item A1. Item i
2. Item ii
• Item B• Item i
• Item 2"

Expected:

• Item 0
• Item 1
  • Item A
    1. Item i
    2. Item ii
  • Item B
    • Item i
• Item 2

Other Information

This PR adds two-space indentation per nested level for ul and ol when using to_plain_text on action text content.

Test case included. All tests passing locally.

@rails-bot rails-bot bot added the actiontext label Dec 15, 2019
@swanson swanson changed the title Better plain text output for nested lists Better ActionText plain text output for nested lists Dec 15, 2019
@kaspth kaspth requested a review from javan December 15, 2019 15:44
@javan javan requested a review from sstephenson December 15, 2019 19:11
@swanson
Copy link
Contributor Author

swanson commented Jan 6, 2020

@javan @sstephenson anything I can do to help move this along?

@swanson
Copy link
Contributor Author

swanson commented Mar 25, 2020

👋 is anyone up for reviewing/giving feedback? Happy to put in more work on this as I still need it for a project and have it monkey-patched at the moment. @kaspth

@kaspth
Copy link
Contributor

kaspth commented Mar 28, 2020

Hey, we've refit Action Text's internals a bunch for a product we're working on, so this code is fairly different. That work is scheduled to go public, but I don't have any details yet. Let's revisit then.

I'll throw it on the milestone for the next major, so we eventually make a call about this. No guarantees as to what the call will be, sorry 😄

@kaspth kaspth added this to the 6.1.0 milestone Mar 28, 2020
@jordanmaguire jordanmaguire mentioned this pull request Apr 6, 2020
Closed
@swanson swanson force-pushed the action-text/plaintext-lists branch from 3bd6eb6 to 0b07fba Compare May 22, 2020 21:18
@swanson swanson mentioned this pull request May 22, 2020
Merged
@rafaelfranca rafaelfranca removed this from the 6.1.0 milestone Nov 24, 2020
Base automatically changed from master to main January 14, 2021 17:01
@rails-bot
Copy link

rails-bot bot commented Apr 14, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Apr 14, 2021
@swanson
Copy link
Contributor Author

swanson commented Apr 14, 2021

Still a valid issue IMO, but seems like it's still pending ActionText internals change?

@rails-bot rails-bot bot removed the stale label Apr 14, 2021
@rails-bot
Copy link

rails-bot bot commented Jul 14, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jul 14, 2021
@swanson
Copy link
Contributor Author

swanson commented Jul 14, 2021

@dhh are you able to share any plans about upstreaming the "refit ActionText internals" from HEY? This issue is still relevant but was pending those potential changes.

@rails-bot rails-bot bot removed the stale label Jul 14, 2021
@rails-bot
Copy link

rails-bot bot commented Oct 12, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Oct 12, 2021
@dhh
Copy link
Member

dhh commented Oct 12, 2021

@swanson We still have plans to upstream Action Text changes, but wouldn't have that prevent doing work here.

@rails-bot rails-bot bot removed the stale label Oct 12, 2021
@swanson
Copy link
Contributor Author

swanson commented Oct 12, 2021

@swanson We still have plans to upstream Action Text changes, but wouldn't have that prevent doing work here.

Thanks, I will rebase and see if I can get the build to pass.

@swanson swanson closed this Oct 12, 2021
@swanson swanson force-pushed the action-text/plaintext-lists branch from 0b07fba to 826f947 Compare October 12, 2021 13:51
@swanson swanson reopened this Oct 12, 2021
@swanson
Copy link
Contributor Author

swanson commented Oct 12, 2021 •
edited
Loading

Merged main, added a few more test cases, tidied up the code a bit, build passing. 👍

@dhh
Copy link
Member

dhh commented Oct 12, 2021

Excellent work!

@dhh dhh merged commit 0b1f83d into rails:main Oct 12, 2021
@swanson swanson deleted the action-text/plaintext-lists branch October 13, 2021 15:10
@tonysm tonysm mentioned this pull request Oct 15, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@javan javan Awaiting requested review from javan

@sstephenson sstephenson Awaiting requested review from sstephenson

Assignees

No one assigned

Labels

actiontext

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@swanson @kaspth @dhh @rafaelfranca