- First commit to issue fork.
- Status changed to Needs review
8 months ago 7:40pm 19 April 2024 To keep consistency between all Field UI tabs, I suggest we add the Machine name column to Manage form and Manage display tables.
I have not found any existing tests related to table headers or table rows' columns thus I have not added new tests. I guess it is not even necessary to check if this column exists ๐คท. If so, please feel free to remove the "Need tests" tag from this issue. If not and we want tests, please help find where I should add such a test.
See attached MR !7612
Capture of the Manage form display with the new Machine name column:
- Status changed to Needs work
8 months ago 8:53pm 19 April 2024 - ๐บ๐ธUnited States smustgrave
Was previously tagged for issue summary update which will need to happen.
As for the tests ManageDisplayTest.php seems like a perfect spot to me. Don't see a test case that perfectly matches so probably could just start a new one.
- Assigned to matthieuscarset
- Issue was unassigned.
Removing "Need tests" tag as relevant test was added to ManageDisplayTest.php.
Pending code review, issue summary update and usability review
- Status changed to Needs review
7 months ago 9:27am 23 April 2024 Issue summary updated.
Marking as NR for Usability and Core reviews.
- ๐บ๐ธUnited States bernardm28 Tennessee
It's an interesting idea and would save some time as one starts theming an entity.
Does it need to be there all the time? Maybe not, but it would be great to have it as a development option or have the ability to unhide it from somewhere on that table. Kind of hidden in plain site per se. - Status changed to Needs work
7 months ago 9:25pm 23 April 2024 - ๐บ๐ธUnited States dww
Love this. Added a note about test efficiency to the MR. I don't think we need a whole new test (and therefore, complete install of Drupal) for 2 assertions. ๐ Let's move these into an existing test to save time / CO2 / DA $.
Otherwise, seems like a win to me, and probably very close to RTBC.
Thanks!
-Derek - Status changed to Needs review
7 months ago 9:30pm 23 April 2024 - ๐บ๐ธUnited States dww
Looking at the rest of
core/modules/field_ui/tests/src/Functional/ManageDisplayTest.php
makes me cringe. Wow that's a lot of separate (tiny) test methods. ๐ข Obviously out of scope to refactor all of this, but probably worth a follow-up.That said, I'm not sure what I'd suggest for this issue, so maybe adding a new test method is fine. Restoring NR, but it smells a little fishy to me. ๐
- Status changed to Needs work
7 months ago 11:59pm 23 April 2024 - ๐บ๐ธUnited States smustgrave
Hiding the patches
Left some comments. Would agree that this could be a follow up for refactoring but think a good task would be to repurpose testSingleViewMode() into testManagerDisplaysInterface or something. Take the comment of the test now "Tests hiding the view modes fieldset when there's only one available." and make it a comment in the test. And obviously update the existing test description.
- Status changed to Needs review
7 months ago 1:13pm 24 April 2024 Thank you for your reviews. I've updated the tests/assertions. Hoping it is acceptable now.
Marking this issue as NR.
- ๐บ๐ธUnited States smustgrave
Applied a small name change to the test.
Rest seems fine to me so +1 but will still need usability.
- ๐บ๐ธUnited States bernardm28 Tennessee
What if Instead of having it's own column it displays as an html subscript
- ๐บ๐ธUnited States smustgrave
Like the idea as we know the machine name will be limited length. May want to wait for UX though. Posted in #ux in slack if anyone frees up
- ๐ซ๐ฎFinland simohell
Having a separate column makes it easier to visually scan labels and machine names. I suppose there are pros and cons to both ways.
- ๐ฉ๐ชGermany rkoller Nรผrnberg, Germany
Usability review
We discussed this issue at ๐ Drupal Usability Meeting 2024-04-26 Active . That issue will have a link to a recording of the meeting.
For the record, the attendees at the usability meeting were @benjifisher, @BlackBamboo, @rkoller, @skaught, and @worldlinemine.
If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.
In general there was a clear consensus that it is convenient to have the machine name right in place. We've considered the point raised in a corresponding discussion in the #frontend channel on the Drupal slack that the
Manage fields
page is already providing the machine names. But on the other hand on theManage form display
page as well as theManage display
page you not only have the machine names for fields but also those for base fields. The machine names for base fields are harder to obtain, requiring turning on the debugging and using some sort ofdump()
function according to @mariohernandez. With the MR in place the machine names for base fields are available as well. So you don't have to switch in-between tabs/pages or open a second browser window anymore.We've also considered the suggestion raised by @bernardm28 in #28 โจ Show machine name in "manage form display" and "manage display" table row Needs review moving the machine name from a dedicated column to the
label
column and display it as an html subscript (plus some aria as he added on the corresponding thread on Slack). That might save one column and horizontal screen real estate but on the other hand moving the machine name as a subscript underneath the label might entail other problems. The subscript would have to be labeled (not only by aria for screenreader users but also for sighted users) and just moving vertically through a column would announce the label and the machine name in a row for each cell. If a screenreader user just wants to get the machine names announced that would be difficult, since the label would always be prepended. So having a dedicated column for machine names requires a bit more horizontal screen real estate, but it is clearer, easier scan-able and less error-prone in particular for screen reader users. Each column is holding specific information and the user is able to navigate by keyboard deliberately retrieving the desired information.We've also taken a look on mobile view ports in combination with extra long labels and machine names. The machine names are getting wrapped same as labels.
So overall we have no concerns with the MR and it looks good to go from our perspective. I'll remove the needs usability review tag again. The only thing I am unsure about is which status should be set. Since the code review task is not crossed of yet i'll leave the status at needs review.
- Status changed to RTBC
7 months ago 9:08pm 26 April 2024 - ๐บ๐ธUnited States smustgrave
Thanks @rkoller and usability team.
All the feedback on the MR has been addressed and is functioning. Test coverage is also there. Believe this is good to go.
- ๐บ๐ธUnited States benjifisher Boston area
xjm โ credited benjifisher โ .
- ๐บ๐ธUnited States BlackBamboo Washington DC
xjm โ credited BlackBamboo โ .
- Status changed to Needs work
7 months ago 9:22am 20 May 2024 - ๐ณ๐ฟNew Zealand quietone
There is a comment that should be changes to be correct. Setting to needs work.
The issue summary has an open item for needing a code review. This was pointed out in #28 as well. Has there been a code review?
- Status changed to RTBC
7 months ago 5:26am 22 May 2024 - Status changed to Needs work
6 months ago 10:17pm 7 June 2024 - ๐ฌ๐งUnited Kingdom longwave UK
Tests are failing.
As a usability fix and UI change this is too late for 10.3.0 now we are in RC, but it is still eligible for the beta phase of 11.0 and also for 10.4.
- Status changed to Needs review
6 months ago 5:37pm 9 June 2024 I don't think failures are related to this code.
All errors seem to be related to something broken in Umami.
Example of a current failure: https://git.drupalcode.org/issue/drupal-3306820/-/jobs/1721378
It's a shame to miss a minor version
10.3
because of an unrelated broken part ๐.How should I proceed to move this issue along?
Sorry I'm not sure.
Rebasing `issue/drupal-3306820:3306820-show-machine-name` over `11.x` ?
This gives a 2k files conflict.
- ๐บ๐ธUnited States smustgrave
Need to rebase with origin/11.x
Current MR is 300+ commits behind which may address the failures
- Status changed to RTBC
6 months ago 7:24pm 9 June 2024 - ๐บ๐ธUnited States smustgrave
Rebased seemed to address the unrelated failures.
Test name appears to be updated.
Code changes make sense for the task being updated.
- ๐บ๐ธUnited States xjm
As a usability fix and UI change this is too late for 10.3.0 now we are in RC, but it is still eligible for the beta phase of 11.0 and also for 10.4.
Actually, it's a UI change, so not beta-eligible for 11.0. Also, UI changes aren't eligible for maintenance minors, which 10.4 is. So I think this is an 11.1 change.
- Status changed to Needs work
6 months ago 9:43pm 17 June 2024 - ๐บ๐ธUnited States xjm
I manually tested this and noticed something that I think needs work:
When you compress the screen to a smaller size, before it hits the breakpoint where the new column would be hidden, the machine names start to wrap, which is something machine names shouldn't do.... but what's worse, it automatically hyphenates the machine names, which is definitely way confusing, because it makes you think the hyphens are a part of the machine name.
So, I think we need to change the word-break or word-wrap behavior for the column, and ensure the column is hidden when some reasonable length of machine name does not fit, and that exceptionally long machine names are elided somehow (which I vaguely remember seeing elsewhere. To suggest a solution, I went to the Manage fields page... and discovered it has the same problem. So, I filed ๐ Fix word-wrapping behavior of machine names in UI tables Active .
- Status changed to RTBC
6 months ago 9:44pm 17 June 2024 - ๐บ๐ธUnited States xjm
Did not mean to NW since I filed the followup myself.
- Status changed to Fixed
6 months ago 9:47pm 17 June 2024 - ๐ณ๐ฟNew Zealand quietone
@xjm, thanks for making the followup. It saves me chasing up 'needs followup' for fixed issues.
Automatically closed - issue fixed for 2 weeks with no activity.
- ๐ฉ๐ชGermany donquixote
I like this change. Thanks everybody!
Just some notes:
- The columns gets even more tight when I click "Show row weights".
I like to do this to manually set row weights to reduce noise in a git commit.
But it is worth it imo. - This needs work in field_group and possibly other contrib modules. Currently it looks broken with field_group enabled.
Again, that's ok with me.
But it deserves a note in the "API changes".
From a usability perspective:
In all the projects I ever worked on, the site builder who manages displays with field_ui is the same person who will then run "drush config:export" and commit the changes in git.
There is no point in hiding machine names from this person. - The columns gets even more tight when I click "Show row weights".
- ๐ฉ๐ชGermany donquixote
It could also be nice to have some kind of feature detection possibility, so that a module like field_group can support different versions.
Currently it could check if any field row has a cell key 'machine_name', but this is not 100% reliable if there are no fields for some reason.