Change from #3172378: Prevent extlink icons from breaking into new lines created problems for our sites due to different extlink behaviour and new html. This is especially bad because we have card like elements that can also have external links and new prevention of extlink icons from breaking into new lines is breaking design more and changing of inner html code is not something we want.
New issue #3476565: Regression: Wrapping of last word in span may break text display, make it optional that is making wrapping optional is fixing our problems because it is basically reverting extlink behaviour as it was working in 1.x. However we also had various problems with extlink icon wrapping for "normal" links and we tried to solve it internally with various approaches, all with limited success. Approach that 2.x branch is doing looks better and it gives us far more flexibility because it is introducing span elements for easy wrapping control.
So ideally we want both behaviors. @berdir got an idea to actually allow new text wrapping for regular links and disable wrapping for more complex like links (like above card links).
Introduce a new option "Wrap only text-like links." which if turned on will prevent wrapping for simple/regular links and will not prevent wrapping for complex links (with complex inner html structure).
We are defining a text-like link as a link that starts with a text and ends with a text and may contain simple HTML elements (such as bold or italic tags) between the text, provided these elements contain no nested children.
NOTE that MR from this issue will be build on top of MR !42 which is still not merged into 2.x.
A new settting UI option "Wrap only text-like links." is introduced as a checkbox. This checkbox is visible only if previous checkbox "Try to prevent text wrapping separating the last word from the icon." is checked.
None.
None.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
pivica commentedComment #4
pivica commentedComment #5
pivica commentedThis is ready for a full review/test. One thing that needs additional thinking is handling of text like links - this are the links that starts and ends with text and that can have simple html elements between text. Simple html elements are elements that do not have additional child elements inside. The idea is that wrapping should work for links like
But complex links like
will not be wrapped.
The questions are edge cases like
Is this a text like link or not, current assumption is that it is not because there are elements on start or end.
Maybe we should just say, link is text like if there is no elements inside of it or it has only elements that have text and no additional inner elements?
Comment #6
smustgrave commentedDefinitely should have test coverage, but general approach seems fine. Though kinda would like to land #3487095: New option "Wrap only text-like links." first since it seems to cover more usages.
Comment #7
berdirThat must land first, this builds on the other issue.
Comment #8
heikkiy commentedJust a small note but the comment from #6 is referencing this issue itself which might confuse users landing directly to this issue. :) I guess it should reference #3476565: Regression: Wrapping of last word in span may break text display, make it optional.
Agreed that this approach seems very useful for card like components etc.
Comment #9
smustgrave commentedOther issue has landed
Comment #10
berdirRebased, still needs tests.
Note: adjusted the update function number, but I noticed that they go on sequentially from the old update functions at the moment. But with the new major version and possibility of multiple minor versions, you might want to consider to switch to something with prefixes matching the new major/minor release, like 200000, 200001 and then 201000 for a 2.1.x release branch. That gives you the ability to add an update function to 2.0.x once 2.1.x is out. Not being prepared for that can end up being very messy.
Comment #11
smustgrave commentedCan start the update version here if you want
Comment #12
smustgrave commentedMinus test coverage anything else missing from this one?
Comment #13
smustgrave commentedTried to get the tests started but they aren't passing so leaving in NW.
Comment #14
smustgrave commentedStill would be a neat feature but js may need work actually.
Comment #16
mdranove commentedUpdated test
Comment #17
smustgrave commentedappears to need a rebase.
Comment #18
mdranove commentedRebased. Issue popped up in the js after rebase that broke a test, fixed that. Back to NR
Comment #19
smustgrave commentedOn my phone but seems a bunch of eslint formatting was reverted.
Also block commenting shouldn’t be used for inline comments.
Comment #20
mdranove commentedok patched the eslint from the artifact and changed the comment to inline.
Comment #21
pivica commentedReviewed all code changes against original MR, mostly tests are added and a lot of ESLINT JS code changes - this are all code style changes, not 100% sure about them, but it is just styling and if core is forcing this style cool then.
Tested against our problematic cards case, still works as expected.
Comment #22
mdranove commentedComment #23
smustgrave commentedComment #26
mdranove commentedWas using this today and was wondering: In the form, since extlink_prevent_orphan is only visible if the extlink_icon_placement is set to 'append', and this new checkbox is only visible if extlink_prevent_orphan is checked, should it also only be visible if the extlink_icon_placement is set to 'append'?
Comment #28
smustgrave commentedlets extend the test coverage for the latest change. There's not been a release yet so I don't need to revert yet.
Comment #29
mdranove commentedGot a simple test for the form state change with some help from chat gpt.
Comment #30
smustgrave commented