Needs work
Project:
Drupal core
Version:
main
Component:
navigation.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
13 Dec 2023 at 14:55 UTC
Updated:
25 Sep 2024 at 06:57 UTC
Jump to comment: Most recent, Most recent file



Comments
Comment #2
m4oliveiComment #3
m4oliveiComment #5
finnsky commentedWe had rude prototype for top bar elements.
https://www.drupal.org/project/navigation/issues/3398483
Probably css and markup of it can be reused
Comment #6
kostyashupenkoJust my thoughts.
1. Each form can have several submit buttons, i.e.: Save, Preview, Something else? Which means we can't just use `for` html attribute with form_id attribute value.
2. Instead we could use somekind of
The solution above is already working, but i don't think it's too much accessible honestly, correct me if i'm wrong.
3. Non else "native" solutions are working.
4. Potentially we could use javascript for this. For everything: creation of the "fake" submit buttons in top bar and for binding listeners.
Few more thoughts but about other thing:
1. I think this task should be not only about moving "Save" button in top bar, but instead we should move all actions of the form in top bar.
2. On which pages we should do this trick? In Drupal there are many pages in admin back office with lots of forms, which has actions. I bet for any admin page where we have form, isn't it ?
3. Potentially one admin page can contain two or more forms (but i can not be sure here) - obviously we shouldn't move all actions from all forms in top bar. What to do in that case?
Comment #7
kostyashupenkoComment #8
ckrinaAnswers to #6:
Ideally, we shouldn't have several Submit buttons. The other ones should be regular buttons and/or links.
See the link on the summary to Stackoverflow where explains that the main button with the attribute
formshould be enough to associate a button to a form-owner in modern web browsers (the ones supported by core).Other thoughts:
Not really: we'll probably more the Delete action somewhere else more hidden to leave more space in the top bar.
This should only happen on Entity forms for now.
Exactly! That's why we should move it only for Entity forms now. I forgot to add it in this specific issue. I've updated the form.
Comment #9
finnsky commentedGonna add POC MR
Comment #11
finnsky commentedMain idea to use form attribute https://dev.to/dailydevtips1/submit-button-outside-the-form-2m6f
Claro buttons has margin so not looking good OOB. :)
We can create custom button button in fact and hide original one.
Comment #13
ckrinaNeeds rebase :)
Comment #14
kostyashupenkoMR rebased
Seems submit is working fine in top bar. But i'm thinking that maybe we shouldn't move original submit, but instead - duplicate? I find it unclear when i'm scrolling down the page and there is no "Save" button in the bottom.
Comment #15
kostyashupenkoComment #18
ckrinaThanks @kostyashupenko! I left a few comments in the MR plus:
This should be removing any default margin the button has to it's adjusted to the top bar and looks minimally OK:
I totally understand this, and the reason is that you're used to find the button there. We discussed this and having the button in 2 places didn't work. We're basically trying to do a change of paradigm here on full page forms.
Comment #19
finnsky commentedBetter to duplicate button then.
We may create Drupal.theme.toolbarButton = () => {}
Or what i personally prefer use web component for this:
https://developer.mozilla.org/en-US/docs/Web/API/Web_components/Using_te...
Comment #20
ckrinaComment #21
aaronmchaleDon't get me wrong, I love the idea here. The challenge we have to consider, I think, is that we would then have the form submit button in very different locations depending on which form you're using.
For instance, if you're on the node edit form you would Save by going to the top right of the screen, but then if you go to a configuration form, you would save by going to the bottom left of the screen, that feels a bit too inconsistent.
There's also the question of whether this is follows form design best practice, the user generally expects to start at the top of the form and work their way down until they get to the bottom where they expect to find the submit button. I suspect (although we would need to test this), that users might find this change challenging, especially users who use magnification software so cannot see the entire screen at once.
Comment #26
nayana_mvr commentedChanges in MR!169 are not getting applied in Drupal 11.x. Getting the following error:-
Tried to apply the changes of MR!7943. Changes applied cleanly but getting console error.
I have created a new MR!9516 incorporating review comments of MR!169. I have placed all the action buttons in top bar otherwise it will be confusing for the editors imo. This is currently working for node edit form, media edit form and taxonomy term edit form.
Regarding,
I think placing the action buttons in top bar is useful so that editor can save the form immediately after editing a field instead of scrolling down the whole form (E.g., if editor needs to edit just the first field value.). Also, top bar is currently available only on entity edit forms. When user is creating a new node/ term/ media, the save button will be in the bottom itself.
Kindly review the changes. Thanks.
Comment #27
finnsky commented@nayana_mvr, original function was written with vanilla js. Is there any special reason to use jQuery?
Comment #28
nayana_mvr commented@finnsky No specific reason. jQuery is my primary language. If required, I will re-write it in vanilla js.
Comment #29
finnsky commentedBetter to avoid jQuery in core contributions. You can find lot of issues in Drupal core about removing jQuery. So new code may contain it only if special reason exists
Comment #30
nayana_mvr commentedThanks @finnsky for the information. I will be careful about that in future. I have update the code using vanilla js. Kindly review.
Comment #31
finnsky commentedFew more ideas we can implement there.
1. We can not copy all buttons with original classes.
Because they contain unexpected styles. Probably it is better to create new buttons with document.createElement('button') there and copy original buttons attributes like https://codepen.io/finnsky/pen/qBzGNoZ?editors=1010
2. We can use existing toolbar button component for that copied buttons. Here you can see idea, https://thunderous-treacle-600c5c.netlify.app/?path=/story/page--basic (Styles can be outdated, this is old POC)
3. Classes for that buttons can be based on button type. Let's say button type="submit" is primary and other buttons just basic.
4. Here you may see original comment https://git.drupalcode.org/project/drupal/-/merge_requests/9516/diffs#71...
We need to do research in Drupal core forms. and find common selector logic for them. I believe should be something. But i'm not sure.
Thank you for participating
Comment #32
nayana_mvr commented@finnsky I have attempted your suggestions and created a patch. Following are the changes done based on the suggestions:-
type=submit' which are 'Save' and 'Preview' buttons in case of node edit form. For 'Delete' action, I cloned the element as it is as I was not sure if it needs to be changed to<button>.class' from original buttons to new buttons and added new classes 'toolbar-button toolbar-button--primary' to the new buttons since both buttons have 'type=submit' (Referred class names from https://thunderous-treacle-600c5c.netlify.app/?path=/story/page--basic).This is how the buttons look like now:-
Regarding the original buttons, should we just hide them from the UI or remove them completely once new buttons are created?
This is just an attempt based on my understanding. As mentioned in previous comment, this may require more research on Drupal core forms.
Comment #33
finnsky commentedSorry, but why patch? before you sent it as MR
Comment #36
nayana_mvr commented@finnsky Sorry for the confusion. Since there could be more changes, I thought creating patch would be better than directly committing to the MR. I have now committed the changes to MR!9516. Please check.