- Issue created by @arthur_lorenz
- @arthur_lorenz opened merge request.
- Status changed to Needs review
about 2 years ago 3:39pm 9 March 2023 - 🇸🇮Slovenia useernamee Ljubljana
Code looks good except for the code style issues, but I guess other files have them to ...
- Status changed to RTBC
about 2 years ago 7:03pm 15 March 2023 - Status changed to Needs work
about 2 years ago 8:38am 17 March 2023 - 🇦🇹Austria fago Vienna
thx, great idea.
However, I don't think it's ok to add this in getLocalTasks() method. It's not a Drupal local-task, what could be confusing if the API returns it as local task then. Then, it's not nice to special case code of individual routes in there. Drupal has APIs for adding local tasks for routes, so if anything we should use them. But that might show up when not using drupal-ce as well.
I think we could just add another custom-element in the node-preview controller and render a link on top of the node-preview, similary as it it's done in a traditional rendered page. So it could either be a a separate custom element, or we just add necessary information the the node-preview custom-element so that the frontend can easily show the link. Maybe a flag to indidicate it's a preview is a enough, i.e. add a attribute "is-preview" ? Then the frontend could show the back-button where suiting.
- 🇦🇹Austria arthur_lorenz Vienna
It's not a Drupal local-task, what could be confusing if the API returns it as local task then.
True. But imo from a user's point of view it is much nicer to have this in the local tasks then rendering an additional link in spot that is only used for this exact link. This link would also need to be taken care of in the frontend to be sure it's styled etc. To me it's equivalent to the
Edit
link in core's local tasks, that's why I want to have it there.Then, it's not nice to special case code of individual routes in there.
Well, the node module basically does the same in a
hook_page_top
. Just the outcome is worse imo.Drupal has APIs for adding local tasks for routes, so if anything we should use them. But that might show up when not using drupal-ce as well.
Exactly, we don't want to interfere with the core functionality here. My goal is to avoid overcomplicating the task. I think the question is whether we want to map Drupal's functionality 1:1, or if we use the gathered flexibility of our custom renderer to add tiny UX improvements like this without having to create a core issue or similar. But the more I think about it I come to the conclusion, that this module might not the right place for this change. Let's discuss it in the next weekly
- 🇦🇹Austria fago Vienna
yep, your arguments convinced me. But I also agree with you that this module is not the right place, lupus_ce_renderer should be an un-opinionated module for ce-renderering. Improving UX with some opinionated solutions fits to lupus_decoupled
Thus, I'd suggest we move this over there. But then, I think we should do it in the light of the whole feature set provided by https://api.drupal.org/api/drupal/core%21modules%21node%21src%21Form%21N... - introducing a way to load previews by view_mode!
Maybe this would be even be better solved in the frontend, e.g. we could add a preview/* route in nuxt which shows the view-mode selection block.