- Notifications
You must be signed in to change notification settings - Fork1.1k
Allow groups of attributes to be set via a toolbar dialog#1197
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
…a groupTagThe only flaw would be that it will fixate on the first matching groupTag so the order attributes are listed in the toolbar dialog data attribute is important.
brendon commentedNov 12, 2024
Ok, so I figured out the issue with not being able to set more than one attribute on a To be honest, it feels like the configuration needs more of an overhaul to cope with this scenario. Ideally acceptable Not sure what your future plans are for configuration but the way it's currently done just isn't flexible enough. Let me know your thoughts. The code now works at least. You can set and unset the target via a checkbox. |
brendon commentedNov 12, 2024
The test suite as it stands now runs green. I'm keen to add some tests to test the new functionality but would like some feedback on the overall PR before I begin that if that's ok? If I've made a mistake in my assumptions then it'd be good to fix that before beginning tests. |
brendon commentedNov 17, 2024
Any chance I could get some feedback? Even if it's just a "yes, good, write some tests" or "think about it this other way". |
brendon commentedDec 10, 2024
@seanpdoyle, I'd love to get some feedback on this PR. I'm not sure where it fits within your greater ambitions but it would be helpful to modify more than one attribute via a dialog. |
brendon commentedFeb 2, 2025
Still keen to get some feedback on this. |
brendon commentedApr 3, 2025
@brunoprietog, I'd love a review of this PR. |
The main point of this PR is to allow one to set
target="_blank"on links on an optional basis (as in the dialog allows the user to tick a box to set that attribute. You'd use the following toolbar dialog:In addition, the ActionText configuration should probably be updated to allow
targetas an attribute:The Problem
This mostly works except for some reason I can't sniff out where some sanitisation is occurring. If I hard code a link in the database content with a
target="_blank"and load it into trix, it strips the target attribute in the HTML it displays in the editor but all of the toolbar tools work just fine. The underlying document model still keeps track of thetargetattribute and the tick box ticks and unticks for a particular link depending on if thetarget="_blank"attribute exists, it's just rendered to the HTML and thus gets lost when serialising too.I'm hoping for some help in solving this last piece of the puzzle and then I'll write some tests to make this PR legit.
#286
#55
To be clear, I'm not arguing for this to be included in the default interface, but it should still be possible to achieve with a custom toolbar and dialog. The aim is to make this change non-breaking (thus the aliased methods).