[#74710] show inline page link macros in editing mode#23142
Conversation
- https://community.openproject.org/work_packages/74710 - show page link as turbo frame - add route for loading the macro - update ckeditor build
|
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
NobodysNightmare
left a comment
NobodysNightmare
left a comment
There was a problem hiding this comment.
This PR LGTM mostly.
Exclusions:
- I did not yet look at the underlying CKEditor changes, those still require a review
- I think we can remove the
providerfrom theInlinePageLinkMacroComponent
NobodysNightmare
left a comment
NobodysNightmare
left a comment
There was a problem hiding this comment.
I left some additional low-profile comments on this PR. They are easier not important or easy to fix.
However, after testing the change locally (now that the previously mentioned error is gone), I found some edge cases in cursor movement. See my comment on the related PR.
- drop before action in controller for more comprehensive code logic
NobodysNightmare
left a comment
NobodysNightmare
left a comment
There was a problem hiding this comment.
Sorry for those two late comments.
In general the PR looks good to me. I think the 🟡 comment should be addressed and the 🟢 comment _could be addressed.
In any case, after fixing what you fixed, you can go ahead with a merge yourself.
| end | ||
| end | ||
|
|
||
| resource :wiki_page_link_macro, controller: "wikis/page_link" do |
There was a problem hiding this comment.
🟢 Last thing that I noticed so late, that it feels weird to bring this up now: I saw that the controller was called PageLinksController, which felt weird to me, because page_links#load could mean anything.
Seeing that the route is already called wiki_page_link_macro improves this a lot for me. I'd still prefer if the controller was equally named wikis/page_link_macro.
There was a problem hiding this comment.
The controller was renamed in the follow up PR - #23193 - to be better distinguished to the other page link related controllers.
| end | ||
| end | ||
|
|
||
| resource :wiki_page_link_macro, controller: "wikis/page_link" do |
There was a problem hiding this comment.
🟡 Another thing that I noticed pretty late in the review, the use of resource adds a bunch of default routes here. None of which you need:
What you want to do is add only: %i[], so that none of the default are auto-generated.
Also see the lines below for this.
There was a problem hiding this comment.
yes, this was a flaw I also detected when working on the next iteration, the PR #23193 has some changes here, changing the route definition to
resource :wiki_page_link_macro, controller: "wikis/inline_page_link_macro", only: [] do
get :load
end
2 participants
Ticket
#74710
What are you trying to accomplish?
What approach did you choose and why?
Related PR
opf/commonmark-ckeditor-build#115