- Issue created by @lauriii
- 🇫🇮Finland lauriii Finland
This was unblocked by 📌 Make field selection a two-step form Fixed .
- 🇺🇸United States benjifisher Boston area
Just to clarify: if the first step of the "Add field" form selects a field type (like Boolean) and not a field group (like Reference), then we should skip the second step and go straight to the field configuration form.
Maybe we should move some code from the submit method of the "Add field" form to the
buildForm
method of the field configuration form. I am not sure how that would affect the "Re-use an existing field" form. - First commit to issue fork.
- Status changed to Needs work
12 months ago 11:44am 10 January 2024 - 🇮🇳India srishtiiee
The second step in case of ungrouped field types mentioned in #3 isn't implemented yet. It will be affected by the issue that removes the radio buttons from the field cards in the first step: 📌 Field selection breaks conventions and increases cognitive load Needs review . I think we should wait on those changes to be committed to avoid re-work here.
- 🇮🇳India omkar.podey
omkar.podey → made their first commit to this issue’s fork.
- 🇮🇳India omkar.podey
The ajax callback was causing the form to be rebuilt which prevented it from being saved the first time, now fixing the issue with the correct order of execution as we should run the validation checks earlier and not let the temp field to be saved early.
- 🇮🇳India omkar.podey
Now the tests are failing because of two main reasons.
- The content translation module somehow doesn't appear to be installed, while works fine in UI.
- The default value field is created with the random temp field name so have to figure out a way to assert that.
- Status changed to Needs review
11 months ago 7:25am 25 January 2024 - Status changed to RTBC
11 months ago 10:55am 25 January 2024 - 🇮🇳India Kanchan Bhogade
Hi,
I have tested MR !5992 on the Drupal version 11.xTesting steps:
- Install Drupal 11.x
- Go to any content type and add a field type like boolean (not a field group like Reference)
- Check for the 2nd step having the Label and machine name
- Apply branch and again check for add field
Test Result:
The patch is applied successfully, and the second step form is skipped and directly goes to the field configuration form for the Boolean type field which does not have a field group like Reference.Attaching screenshots for reference.
Moving to RTBC
- 🇩🇪Germany rkoller Nürnberg, Germany
hm i've tried to apply the patch to a completely fresh install of drupal 11.x-dev installed with the standard profile. but somehow i get
Could not apply patch! Skipping. The error was: Cannot apply patch https://git.drupalcode.org/project/drupal/-/merge_requests/5992.diff
when i try to apply the diff with composer patches. but "somehow" some" changes or all(?) changes got applied. but not sure if that is an actual problem with the patchor a problem due to the way composer-patches applies the patch, just wanted to note.but changes are in place, at least in the UI, so i was able to give it a quick try. definitely the right choice moving the label/machine_name field over to the field config edit form. looks great!
one detail to note. 📌 [PP-1] Replace the "Change field type" button with one that goes to the previous step Postponed which is currently postponed on this issue is a sort of essential addition to this issue from my perspective. because currently the only way to move one step back as soon as you are on the field config edit form is the back button of your browser. there should definitely be a dedicated option to navigate back and forth in the interface through all the steps until the field is finally saved in this last step. otherwise one might feel stuck arriving on this field config edit form in case the person had clicked the wrong field type or option in one of the previous steps.
- 🇩🇪Germany rkoller Nürnberg, Germany
Another detail I've just noticed. For the field type
Media
you directly get to the field settings form now. "problem" is likeField upload
pre patch it had a description underneath explaining when to use media reference fields and when to use file and image reference fields. ForField upload
that description is still shown on step two when you are able to choose the option, that second step got removed forMedia
and so is the description. I am not sure if it would make sense to squeeze it onto the field settings page instead for media. but i wonder if it would make sense to create a dedicate help topics page in a follow up where the user gets an overview of all available fields in core and when best to use which field. that you have all available options and background information for each field type on a single page. - Status changed to Needs work
11 months ago 6:09am 26 January 2024 - 🇺🇸United States benjifisher Boston area
@Kanchan Bhogade: Thanks for testing, but please do not move an issue to RTBC unless it has also passed code review. The R is for review and the T is for test.
@rkoller: I am moving the issue back to NW for Comment #13. Losing the notice about media and image fields is a regression. I am adding a note to the issue summary so that we do not lose track of this regression.
I do not think we have to worry about applying the diff as a patch (Comment #12). You should be able to clone the issue fork and check out the feature branch using the instructions near the top of the issue,
I think it is OK to keep 📌 [PP-1] Replace the "Change field type" button with one that goes to the previous step Postponed as a separate issue. If we can get this issue fixed, then I think it is a step in the right direction even without that improvement. I hope we can get both done in time for Drupal 10.3.
- 🇫🇮Finland lauriii Finland
I am moving the issue back to NW for Comment #13. Losing the notice about media and image fields is a regression. I am adding a note to the issue summary so that we do not lose track of this regression.
I'm not 100% sure the message is needed for the media type at least when not editing fields for media entity. Maybe on this issue we move the message to the field config edit form and discuss in a follow-up if we should not display it when editing fields for other entity types than media.
- 🇩🇪Germany rkoller Nürnberg, Germany
I am not sure if the field settings edit form is the best place even as a temporary place for that description. That form and page is the last step in the field creation flow, step 2 for media and step 3 for file upload with this patch applied. that description ideally would be more useful at the point where the person trying to create a field and either doesn't know anything about the different field types or is uncertain which field type is the right pick, in case of the media and file upload example, and needs a reminder.
i wonder if it would make sense to remove both the descriptions for media and file upload in step 2 and 3. instead incorporate the brief gist into their short descriptions in step one (i am currently working on a draft for 📌 Refine field descriptions Active ). in addition to that provide a description pointing to a to be created help topic page providing an overview for a "when to pick which field type" situation. But I would place the description pointing to the help topics page before the list of available field types in step 1 instead of after. - 🇮🇳India omkar.podey
Then maybe something like this
could work, just having the general message appear on the initial field selection whenever media is enabled. - Status changed to Needs review
11 months ago 10:25am 6 February 2024 - 🇮🇳India djsagar
After applied MR !5992, Message appear bottom of the choose field type.
Before MR
After MR
RTBC ++.
- Status changed to Needs work
11 months ago 5:58pm 6 February 2024 - 🇺🇸United States bnjmnm Ann Arbor, MI
I'm not sure about adding info to the bottom of this form specific to choosing media. The options themselves already include description text specific to that option - and option-specific descriptions should live there. Ideally the current media description text could be shortened (its longer than it needs to be IMO) to make room for this info. Having it as a footnote technically addresses the regression, but I don't think it actually improves the experience.
If rewriting that description is not a feasible option, at the very least associate the bottom-of-the-form text with the media option with an asterisk. In its current state I think it is unlikely a user will even read it - they will see the option they want then click it to proceed.
- 🇮🇳India omkar.podey
Does this seem better and attracts enough attention to the note?
- Status changed to Needs review
10 months ago 6:45am 14 February 2024 - Status changed to Needs work
10 months ago 12:41pm 14 February 2024 - 🇺🇸United States bnjmnm Ann Arbor, MI
Does this seem better and attracts enough attention to the note?
It's better than not having the association, but it's not great. I think updating the descriptions has a better result as it keeps the UI clean, removes the edge case, and the relevant information is in the correct context. Also less code needed 🙂
Something like this looks better and I think has a higher chance of benefitting a user vs. the current tacked on message.
- 🇺🇸United States phenaproxima Massachusetts
This stuff is really tricky to understand, so nice work @omkar.podey! That said, I did a code review and found some stuff that should probably be fixed, plus some questions that need answering.
- Status changed to Needs review
10 months ago 3:06pm 20 February 2024 - 🇺🇸United States smustgrave
See a lot of conversation in the threads and feedback (60 threads!) This good for review now?
- 🇺🇸United States tim.plunkett Philadelphia
I've asked @omkar.podey to mark resolved threads with ✅ so @srishtiiee can close them
- 🇺🇸United States smustgrave
So testing was pretty straight forward. Applied the MR and verified label is appearing on config edit form.
Tested with a few field types (had 0 doubt this would matter but double checked) and no issue creating fields.
Remaining tasks appear to be complete.
Issue summary contains a screenshot of the new change.+1 from me but may be worth getting additional eyes.
- Status changed to RTBC
10 months ago 8:10pm 10 March 2024 - 🇺🇸United States smustgrave
From what I can tell all feedback has been addressed.
- Status changed to Needs review
9 months ago 5:28am 13 March 2024 - 🇺🇸United States smustgrave
Appears to be a few open threads or is this ready for testing?
- Status changed to RTBC
9 months ago 3:06pm 2 April 2024 - 🇺🇸United States smustgrave
Rebased the MR as thought it had been a while, nothing broke
Retested following same steps
Applied the MR and verified label is appearing on config edit form.
Tested with a few field types (had 0 doubt this would matter but double checked) and no issue creating fields.Believe all threads have been fixed or answered
For me this looks good.
- Status changed to Needs work
9 months ago 6:28am 4 April 2024 - 🇬🇧United Kingdom catch
There are unresolved threads on the MR for things that are very confusing to me.
- Status changed to Needs review
9 months ago 10:53am 4 April 2024 - 🇮🇳India omkar.podey
I simplified the whole code block by removing the creation of new field storage config and hence moving away from the whole
translatable
key issue. - 🇺🇸United States smustgrave
Seems to be 1 open question left a comment. Once answered don't mind marking
- Status changed to RTBC
8 months ago 3:06pm 22 April 2024 - 🇺🇸United States smustgrave
Keep this one from stalling going to mark as it's still functional.
- Status changed to Needs work
8 months ago 9:14pm 22 April 2024 - 🇬🇧United Kingdom catch
Replied on the MR - but this shouldn't have been RTBC again, the thread is still outstanding.
- Status changed to Needs review
8 months ago 7:15am 26 April 2024 - Status changed to Needs work
8 months ago 2:25pm 28 April 2024 - Status changed to Needs review
8 months ago 9:42am 29 April 2024 - Status changed to RTBC
8 months ago 1:04pm 29 April 2024 - 🇺🇸United States xjm
With this patch applied, I'm getting a weird behavior where clicking "Save" on the field configuration form the first time causes the page to jump back to the top, and I have to click "Save" a second time in order to save the configuration. It does not seem to happen with HEAD. Steps to reproduce:
- Install Standard on 11.x HEAD with the patch/MR applied.
- Go to
admin/structure/types/manage/article/fields
- Click "Create a new field".
- Select "Boolean" (it occurs for other field types, but this one's simplest)
- Enter a label.
- Click "Save". Note that the cursor jumps to the top of the page.
- Click "Save" a second time and note that the field is only then saved.
I've been able to reproduce this consistently with the patch, and able to consistently reproduce it not happening with HEAD.
- 🇺🇸United States smustgrave
Actually seeing the same as @xjm appears the code that runs the machine name could be causing it.
If I enter a label
Click out of the label input (any where on the page)
Click save I don't get that behavior. - 🇺🇸United States xjm
I also disagree that losing this message is an acceptable regression or scopable to a followup:
This message was an important compromise for the stabilization of Media, to dissuade people from creating a bunch of difficult-to-migrate image data when they should have been using Media. That single paragraph was the compromise for giving up built-in redirects, a migration tool, removal of image fields from the UI except on Media types, etc. etc. etc. It does not have the greatest UX -- ideally it would be styled as a little info box rather than a blah paragraph of text, or something even more assertive -- but that doesn't mean it's okay to lose it entirely.
- Status changed to Needs work
8 months ago 12:27am 30 April 2024 - 🇺🇸United States xjm
I confirmed what @smustgrave said above: If you click out of the label field, the bug does not happen. (Clicking in or out of the label field in HEAD had no effect; it worked every time.)
I also reproduced #47 in Firefox, both it not happening in HEAD, and it happening with the patch applied.
- 🇺🇸United States xjm
Another regression is in the page title for the field configuration page. In HEAD we have:
With the patch it's simply:
This sounds like it's all field settings for articles, rather than the settings of one particular field.
I understand that we don't have the field name when the page is loaded, but could we at least put the field type in the title? E.g.:
Boolean field settings for Article
- 🇺🇸United States xjm
I finally finished my manual testing and my code review -- phew! Overall this is in pretty good shape, and it's a really good usability improvement on the whole. IMO there are three regressions that need addressing (see #47, #49, and #51), plus a few small cleanups, but on the whole it's close to ready. Great work everyone!
Saving issue credits.
- 🇺🇸United States xjm
Tagging per my comment above about the temporary label and machine name being generated to make the form work.
- 🇬🇧United Kingdom catch
Re #53 I don't have a better idea, but I found that incredibly confusing reading the MR (like deleting config entities that would never actually exist etc.). Definitely could use a follow-up to revisit. I haven't reviewed this since my last round of feedback was addressed.
- Status changed to Needs review
8 months ago 7:15am 7 May 2024 - Status changed to Needs work
8 months ago 11:56pm 8 May 2024 - 🇺🇸United States benjifisher Boston area
The screenshot in the issue summary needs an update. The page title now includes the field type. For example, when adding a boolean field, the page title is "Boolean settings for Basic page Add to Default shortcuts", not "Field settings for Basic page Add to Default shortcuts".
After I save the new field, I can still edit it. When I do that, the page title still includes the field type (like "Boolean") instead of the field name (like "Test boolean field"). The same thing happens with existing fields. For example, with the Standard profile I can visit
/admin/structure/types/manage/page/fields/node.page.body
, and I see the page title "Text_with_summary settings for Basic page". I think we should use the field name when it is available.I also left some comments on the MR. We need the usual BC steps when adding a parameter to the constructor, and that means we need a change record. I am adding the issue tag for that.
- Status changed to Needs review
7 months ago 8:59am 22 May 2024 - Status changed to Needs work
7 months ago 2:51pm 4 June 2024 - 🇺🇸United States smustgrave
Looking at the tags and can't answer the release or framework manager. But can the issue summary be updated from #56 (should help the manager review later).
Left a small comment on MR. Also appears to have a number of test failures. Some of them could need a rebase but didn't check each.
- 🇺🇸United States benjifisher Boston area
Usability review
We discussed this issue at 📌 Drupal Usability Meeting 2024-11-08 Active . That issue will have a link to a recording of the meeting.
For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @rkoller, @shaal, @simohell, @worldlinemine.
We agreed that, of all the child issues of 🌱 [Plan] Improve field creation experience Active , either this issue or 📌 Move the first two steps of field creation into a modal. Needs work would be a good next step.
Experience shows that site builders frequently miss the required Label field (and the related, hidden by default, machine name field) on the current form. Moving it to the next step in the process would be an advantage. A further step (for some future issue) would be to go to the next step after choosing a field type in one step (select an option) rather than two steps (select an option, then submit the form).