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

Allow groups of attributes to be set via a toolbar dialog #1197

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

brendon
Copy link

@brendon brendon commented Nov 11, 2024

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:

<div class="trix-dialogs" data-trix-dialogs>
  <div class="trix-dialog trix-dialog--link" data-trix-dialog="href" data-trix-dialog-attributes="href target">
    <div class="trix-dialog__link-fields">
      <input type="url" name="href" class="trix-input trix-input--dialog" placeholder="${lang.urlPlaceholder}" aria-label="${lang.url}" required data-trix-input>
      
      <input type="checkbox" value="_blank" id="href_target" name="href[target]" data-trix-input>
      <label for="href_target">Open in a New Tab?</label>
      
      <div class="trix-button-group">
        <input type="button" class="trix-button trix-button--dialog" value="${lang.link}" data-trix-method="setAttribute">
        <input type="button" class="trix-button trix-button--dialog" value="${lang.unlink}" data-trix-method="removeAttribute">
      </div>
    </div>
  </div>
</div>

In addition, the ActionText configuration should probably be updated to allow target as an attribute:

ActionText::ContentHelper.allowed_attributes.add 'target'

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 the target attribute and the tick box ticks and unticks for a particular link depending on if the target="_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).

…a groupTag

The 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
Copy link
Author

brendon commented Nov 12, 2024

Ok, so I figured out the issue with not being able to set more than one attribute on a groupTag and it's to do with how createContainerElement() worked. It would stop at the first attribute that had a groupTagName configuration and then create an element with that tag and just the attribute that had the groupTagName. I've modified that so that it keeps going through the other elements to see if there are any other attributes with the same groupTagName and it'll add those attributes when the new element is made.

To be honest, it feels like the configuration needs more of an overhaul to cope with this scenario. Ideally acceptable siblingAttributes should be defined on the href configuration but we also need to change code that tries to identify if an attribute is a text or block attribute.

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
Copy link
Author

brendon commented Nov 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
Copy link
Author

brendon commented Nov 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
Copy link
Author

brendon commented Dec 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.

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.

1 participant