feat/content text wysiwyg - #32
Merged
Merged
Conversation
Contributor
Reviewer's GuideAdds an Unfold-styled Trix WYSIWYG editor for the Fobi Sequence diagram for applying WYSIWYG widget on content_text form initialisationsequenceDiagram
actor User
participant BasePlugin
participant ContentTextForm
participant InlineToolbarWysiwygWidget
User->>BasePlugin: get_initialised_edit_form_or_404(slug)
BasePlugin->>ContentTextForm: __init__(*args, **kwargs)
activate ContentTextForm
ContentTextForm->>ContentTextForm: patched_init(*args, **kwargs)
ContentTextForm->>ContentTextForm: force_wysiwyg(self)
ContentTextForm->>ContentTextForm: fields.get(text)
ContentTextForm->>InlineToolbarWysiwygWidget: InlineToolbarWysiwygWidget()
deactivate ContentTextForm
BasePlugin-->>User: ContentTextForm(instance) with InlineToolbarWysiwygWidget
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The inline toolbar template uses a fixed
id="trix-toolbar"anddocument.querySelector('template[id="trix-toolbar"]'), so with multiple WYSIWYG fields on the same page the first script will remove the template and later instances won't find it; consider making the template ID unique per widget (e.g. includewidget.nameor the field ID) and querying relative to the widget container instead of the global document. - In
InlineToolbarWysiwygWidget.get_context,toolbar_buttons_idis derived directly fromnameand then used as a DOM ID; to avoid invalid or clashing IDs when field names contain characters like dots or brackets, it would be safer to derive this fromattrs['id']or runnamethrough a sanitizing helper (similar to Django’sauto_idhandling).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The inline toolbar template uses a fixed `id="trix-toolbar"` and `document.querySelector('template[id="trix-toolbar"]')`, so with multiple WYSIWYG fields on the same page the first script will remove the template and later instances won't find it; consider making the template ID unique per widget (e.g. include `widget.name` or the field ID) and querying relative to the widget container instead of the global document.
- In `InlineToolbarWysiwygWidget.get_context`, `toolbar_buttons_id` is derived directly from `name` and then used as a DOM ID; to avoid invalid or clashing IDs when field names contain characters like dots or brackets, it would be safer to derive this from `attrs['id']` or run `name` through a sanitizing helper (similar to Django’s `auto_id` handling).
## Individual Comments
### Comment 1
<location path="src/unfold_fobi/templates/unfold_fobi/forms/wysiwyg_inline_toolbar.html" line_range="10" />
<code_context>
+{% include toolbar_template %}
+{{ toolbar_buttons|json_script:toolbar_buttons_id }}
+
+<input type="hidden" name="{{ widget.name }}" id="wysiwyg-{{ widget.name }}"{% if widget.value != None %} value="{{ widget.value }}"{% endif %} {% include "django/forms/widgets/attrs.html" %}>
+
+<div class="max-w-4xl relative">
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid duplicating the `id` attribute on the hidden input when `attrs` already provides one.
Including `django/forms/widgets/attrs.html` here can emit a second `id` when `attrs` already defines one, resulting in invalid HTML and brittle JS selectors. Either remove the hardcoded `id` and rely on `attrs`, or strip `id` from `attrs` for this element and only reuse it on `<trix-editor>`.
</issue_to_address>
### Comment 2
<location path="src/unfold_fobi/patches/content_text_wysiwyg.py" line_range="53-59" />
<code_context>
+ return form
+
+ # --- Patch ContentTextForm.__init__ for direct instantiation paths ---
+ original_init = content_text_form_class.__init__
+
+ def patched_init(self, *args, **kwargs):
+ original_init(self, *args, **kwargs)
+ force_wysiwyg(self)
+
+ content_text_form_class.__init__ = patched_init
+ content_text_form_class._wysiwyg_widget_applied = True
+ # Sentinel that short-circuits apply_widgets' lazy __init__ re-wrap.
</code_context>
<issue_to_address>
**suggestion:** Preserve the original `__init__` metadata when monkey-patching `ContentTextForm`.
The replacement `__init__` drops metadata such as `__name__`, `__qualname__`, and `__doc__`, which can hinder debugging and introspection. Wrap `patched_init` with `functools.wraps(original_init)` to retain these attributes without changing behavior.
Suggested implementation:
```python
original_init = content_text_form_class.__init__
@functools.wraps(original_init)
def patched_init(self, *args, **kwargs):
original_init(self, *args, **kwargs)
force_wysiwyg(self)
```
Ensure `functools` is imported at the top of this module, e.g.:
- `import functools`
If the file does not yet import `functools`, add that import alongside the other standard-library imports.
</issue_to_address>
### Comment 3
<location path="README.md" line_range="264-267" />
<code_context>
+
+```python
+# Restrict which buttons appear (subset of the names below).
+# Available: p, underlined, bold, italic, strike, link,
+# heading1, heading2, heading3, heading4,
+# quote, code, bullet, number, indent, outdent, undo, redo.
</code_context>
<issue_to_address>
**suggestion (typo):** Consider confirming whether `underlined` should be `underline` for the toolbar button name.
`underlined` is inconsistent with the other labels (`bold`, `italic`, `strike`, etc.). If the actual config value is `underline`, please update this entry to match and avoid confusion.
```suggestion
# Restrict which buttons appear (subset of the names below).
# Available: p, underline, bold, italic, strike, link,
# heading1, heading2, heading3, heading4,
# quote, code, bullet, number, indent, outdent, undo, redo.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
… add `functools.wraps` to patch decorator
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add Unfold-styled WYSIWYG support for the Fobi content_text plugin and adjust related admin UI and docs.
New Features:
Enhancements:
Documentation:
Chores: