- 🇮🇪Ireland lostcarpark
There seems to be a lot of overlap between this and ✨ Trim paragraph containing word count Closed: duplicate . It would be worth reviewing both as they both add a very similar option. This change seems to explicitly consider paragraphs in the splitting, which I don't think are part of the other.
Regarding the paragraph split, it seems to only use the
<p>
tag. I think it would be valid to include a list of paragraph divider tags, as headings, lists or other common types are not currently considered.The change also seems to have sentence breaks hard coded. It would be good to use the sentence break characters constant defined 🐛 Error cropping Japanese Needs work , when that change gets merged.
- First commit to issue fork.
- @mjgruta opened merge request.
- @mjgruta opened merge request.
- Merge request !60Issue #2815981: Add ability to trim at sentence break when using words → (Open) created by mjgruta
- Status changed to Needs work
over 1 year ago 10:43am 9 July 2023 - 🇺🇸United States ultimike Florida, USA
@mjgruta,
We appreciate your efforts on this issue, but I'm setting it back to "Needs work" for a couple of reasons:
- There is no test coverage in the MR. All bug fixes and feature additions to the Smart Trim module require test coverage.
- I don't think we have consensus on whether or not "sentence" should be a new trim unit (similar to "characters" and "words") or the functionality should be added via a separate checkbox (as the current MR does).
I'm not crazy about the logic of the current MR, as if the formatter is configured for a 50-character trim, but the sentence is 200 characters (not uncommon), then the trimmed text is going to be 4 times longer than the 50-character settings. I think this will lead to undesired results in a non-trivial number of situations. Therefore, I'd like to see this functionality implemented using "sentence" as a new trim unit. This would also have the added benefit of not adding a new checkbox to the formatter config UI.
-mike
- 🇮🇪Ireland lostcarpark
I can understand attaching a patch file might be useful for people to apply to live sites, but shouldn't the patch be taken from MR !60 rather than the previous #15 patch?
- First commit to issue fork.
- Status changed to Needs review
10 months ago 5:20am 4 March 2024 - 🇵🇭Philippines mjgruta
Updates from #30 break the patch. I have merged the latest code.
- Status changed to Needs work
10 months ago 5:40am 4 March 2024 - 🇵🇭Philippines mjgruta
I just noticed the comment from mike. Reverting the status to Needs work.
- 🇮🇪Ireland lostcarpark
I've made a few suggestions.
I think we also need some test cases that explicitly verify this.
- 🇵🇭Philippines mjgruta
Created a diff from commit 0779410f while fixing MR #60 to prevent the composer from failing to apply the patch.
- 🇵🇭Philippines mjgruta
Created another patch file from commit 975c9a47. This should fix the comments from James.
- 🇺🇸United States joshf
This patch fixes the fatals caused by the previous change.
- 🇵🇭Philippines mjgruta
@josh Sorry, the fix needs to be done on the field formatted viewElements as it is trying to pass the $item->format directly which will return null if the item doesn't have a format.
I have updated the code. - 🇺🇸United States joshf
Hi @mjgruta, thanks; your latest change fixes the fatals I was getting with the previous one.
- 🇺🇸United States joshf
This fixes an error that popped up in our automated testing.
- 🇷🇺Russia Andrew Answer Novosibirsk
+1 to release this feature in the new module version.