Convert to JS Widget#18
Conversation
| } | ||
|
|
||
| #dev-toolbar-toggle { | ||
| all: unset; |
There was a problem hiding this comment.
I changed the toolbar icon from a link to a button, since clicking on it doesn't navigate to another (part of the) page. all: unset resets the default styling of the button.
| def show | ||
| @erd_path = Rails.root.join("erd.png") | ||
| render :show | ||
| render "dev_toolbar/erd/show", formats: [:html] |
There was a problem hiding this comment.
The Rails app was not finding this view template until I specified it with this default. I don't this this should be necessary, but I wasn't able to figure it out. I'd love to learn why the previous way wasn't working.
There was a problem hiding this comment.
I also looked into this for a bit, but wasn't able to figure it out. Claude 4.7 sent me down some paths, which were all wrong in the end. This is a weird one, but I'm going to leave it alone now.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cd1f924bf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| links.classList.toggle('hidden'); | ||
| }); | ||
| </script> | ||
| <meta name="dev_toolbar_config" content='#{toolbar_links_content}'> |
There was a problem hiding this comment.
The configured toolbar links needed to be passed from the initializer to the JavaScript file somehow so I decided to store them in a meta tag as JSON. Since it's in the <head> it won't clutter up the <body>, which is where students will mostly be looking.
There was a problem hiding this comment.
Yeah, this is very small and discrete in the source.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29ae63b3c7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9eb1aae206
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| </div> | ||
| </div> | ||
| ` | ||
| document.body.insertAdjacentHTML('beforeend', toolbar_html) |
There was a problem hiding this comment.
Make toolbar render idempotent on repeated Turbo loads
Toolbar.render() always appends a new #dev-toolbar node with insertAdjacentHTML, so after Turbo restores a cached page (for example, browser Back/Forward) and fires turbo:load again, the page accumulates duplicate toolbars and duplicated toggle controls. This regression was introduced by moving rendering client-side without a guard/removal step; add an existence check (or remove before cache) so repeated loads do not duplicate UI.
Useful? React with 👍 / 👎.
| }); | ||
| } | ||
|
|
||
| document.addEventListener("turbo:load", function() { |
There was a problem hiding this comment.
Do we always expect the turbo:load event? Should we also listen for DOMContentLoaded, in case turbo:load does not occur?
I still think the main thing to optimize is minimizing the impact (lines of code added) to the source HTML page. The number of files to configure for the gem to work is less important in my mind because none of our students are going to manually install this gem. All projects and the template will have it pre-installed and configured for them. On our end, we can write a script or installer to make that process easier to make new projects and/or trigger project syncing if there are too many steps right now. That said, we can probably condense the importmap assets into one file, but I think the CSS should be loaded through the asset pipeline of the Rails app instead of being added through the middleware. I intentionally made that change since injecting a style tag with middleware will cause it to appear in the source HTML, which we're trying to avoid. Before I make any changes, what are your thoughts on this? |
I agree with your reasoning here. I was thinking it would be more lightweight to maintain and hook into projects with my update, but I don't think we need to worry much about that. I'll close my experimental PR and delete that branch. I left one question above about |
Resolves https://github.com/firstdraft/appdev/issues/751
Problem
Solution
This PR updates the dev_toolbar to now be injected into the DOM via JavaScript. Viewing the page source in the Rails will no longer show the style tag and HTML for this gem.
Review notes
Take any AD1 Rails app and: