Allow multiple dialogs by using the default dialog mechanism in trix#1409
Allow multiple dialogs by using the default dialog mechanism in trix#1409simmerz wants to merge 12 commits intoSpinaCMS:mainfrom
Conversation
|
Great! I believe the original |
|
It was indeed! It just doesn't fit this purpose, and Trix has its own dialog management, so why tinker :) |
spina.gemspec
Outdated
| gem.add_dependency "jsonapi-serializer" | ||
| gem.add_dependency "browser" | ||
| gem.add_dependency "tailwindcss-ruby" | ||
| gem.add_dependency "tailwindcss-ruby", '< 4.0' |
There was a problem hiding this comment.
This makes sense, but it does break projects using Tailwind v4. I should probably upgrade Spina to use Tailwind v4 as well.
There was a problem hiding this comment.
It doesn't CI properly unless that's added in at the moment. It does break, and I don't like it being there.
There was a problem hiding this comment.
I think the right solution here is to remove this actually, and to upgrade TW in a separate PR, then rebase this and confirm all works and merge.
simmerz
force-pushed
the
multiple_trix_dialogs
branch
from
95a4b9f to
98521be
Compare
simmerz
force-pushed
the
multiple_trix_dialogs
branch
from
93d8366 to
d02597d
Compare
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the Trix editor's dialog system to support multiple dialogs by reverting to Trix's native dialog mechanism. The main change replaces the custom reveal controller-based dialog with Trix's built-in dialog attributes and styling.
Changes:
- Replaced custom dialog implementation with Trix's native dialog mechanism in the toolbar component
- Added CSS styling for Trix dialogs with proper positioning and visibility control
- Removed max-width constraint from modal windows
- Added generated Tailwind CSS file to .gitignore
- Fixed trailing whitespace and formatting issues across multiple files
Reviewed changes
Copilot reviewed 3 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
app/components/spina/forms/trix_toolbar_component.html.erb |
Refactored link dialog to use Trix's native dialog attributes instead of custom reveal controller |
lib/generators/spina/templates/app/assets/stylesheets/spina/application.tailwind.css.tt |
Added Trix dialog styling with positioning, visibility rules, and updated modal-window class |
app/assets/builds/spina/tailwind.css |
Compiled Tailwind CSS output (auto-generated file) |
.gitignore |
Added generated CSS file to ignore list |
.github/workflows/ruby.yml |
Fixed YAML formatting and added package update to apt-get |
| Other ERB files | Removed trailing whitespace |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -webkit-backdrop-filter: blur(10px); | ||
| backdrop-filter: blur(10px); | ||
| @apply w-full max-w-lg overflow-hidden relative; | ||
| @apply w-full overflow-hidden relative; |
There was a problem hiding this comment.
The max-width constraint has been removed from the modal-window class, which could cause modals to become excessively wide on large screens. Consider adding a max-width value (e.g., max-w-2xl or max-w-3xl) to maintain reasonable dialog proportions across different screen sizes.
| @apply w-full overflow-hidden relative; | |
| @apply w-full max-w-3xl overflow-hidden relative; |
|
|
||
| .btn-primary:hover { | ||
| @apply bg-spina-dark; | ||
| } |
There was a problem hiding this comment.
This comment listing multiple Tailwind classes appears to be a workaround to force the Tailwind JIT compiler to include these classes in the build. Consider documenting why these specific classes are needed and where they're used, as this is non-obvious and could lead to confusion. A better approach would be to use Tailwind's safelist configuration if this is truly necessary.
|
Continued in #1418 |
Slight design change in the link modal to stop it overlapping other editors.
Primary focus of this PR is to allow multiple buttons to have dialogs by reverting back to the native mechanism in trix.
I've had to add a comment with some tw classes, to force the tailwind builder to see them. Happy to remove, but didn't know a better way to handle that whilst still on older versions of tailwind-ruby/rails and not using the new engine features in importmaps.