Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
The description for the Inline Form Error moduled on the Extend page does not follow the Help text standard as described in https://www.drupal.org/node/632280
Proposed resolution
Edit the text into one concise sentence by removing the second sentence.
Since this module is listed in the section "Experimental", the second sentence can just be deleted.
This issue should be tagged with the module, but that's not yet in the list of components.
Remaining tasks
none
User interface changes
none
API changes
none
Data model changes
none
Comment | File | Size | Author |
---|---|---|---|
#36 | 2702545-inline-form-error-description-32.patch | 671 bytes | ifrik |
#32 | interdiff-2702545-14-32.txt | 603 bytes | ifrik |
#32 | edit_the_inline_form-2702545-14.patch | 632 bytes | ifrik |
#30 | screenshot-fpp nightingale 2016-05-24 20-29-52.png | 15.47 KB | ifrik |
#18 | after.png | 21.72 KB | snehi |
Comments
Comment #2
ifrikI've shortened the module description by removing the second sentence saying that the module is experimental because it's already in the section of experimental module and to keep it consistent with the Help text standard.
Explanations about what still might not work or why could be better placed in the help text.
Comment #3
ifrikThe previous patch included composer file changes.
Comment #5
NitinSP CreditAttribution: NitinSP at Clarion Technologies commentedComment #6
NitinSP CreditAttribution: NitinSP at Clarion Technologies commentedI have remove second line from description of .info file.Please review the latest patch #6.
Comment #7
Sonal.Sangale CreditAttribution: Sonal.Sangale at Blisstering Solutions commentedComment #8
catchThis is a warning to people that might install the module, since it's pretty broke at the moment. If we remove it from here, we need it somewhere else. Or we should consider retiring this since there's no active work on it and moving it back to a patch in the queue.
Comment #9
ifrikNitinSP,
your patch includes the exact same text change as I've already done in my patch #3.
Why are you doing this?
Comment #10
ifrikThanks Catch,
This module is already described as an "Experimental module" and therefore users need to confirm that they really want to install it and get yet another warning after the installation. Other experimental modules are also listed as "alpha" on https://www.drupal.org/core/experimental but don't have this additional warning.
If the module is in such a state that it really needs so many additional warnings, then it probably really should not be there.
However, if it does stay in core, then it would be more useful for sitebuilders who want to try out the module to know better what functionality might not work. This could be added to the hook_help text.
Comment #11
NitinSP CreditAttribution: NitinSP at Clarion Technologies commented@ifrik
As per comment #4 your patch is failed and this issue is unassigned, so that I created that patch.
Comment #12
ifrikNitinSP,
if a previous patch fails, then you should work on that patch - not starting from scratch, and you should not rename the patches in an issue.
However, in this case the test bot had apparently failed when the test run the first time, but it then successfully passed when it run again, as you can see in the status.
Comment #13
sugaroverflow CreditAttribution: sugaroverflow commentedI working on this at Drupalcon New Orleans.
Comment #14
sugaroverflow CreditAttribution: sugaroverflow commentedComment #15
aburrows CreditAttribution: aburrows as a volunteer commentedTested this and patch applied successfully. Awaiting test then RTBC
Mentored sugaroverflow on this issue / patch.
Comment #16
YesCT CreditAttribution: YesCT commentedThe screenshot should probably be of the module page. Before and after would be nice.
Comment #18
snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commentedDone the testing working fine.
Adding screenshots.
Before the patch:
After the Patch
Comment #19
aburrows CreditAttribution: aburrows as a volunteer commentedSweet, tested again locally and its working fine for me RTBC++
Comment #20
aburrows CreditAttribution: aburrows as a volunteer commentedComment #21
xjmThanks @ifrik and sprinters.
Unfortunately, these other modules are not broken in the way Inline Form Errors is. They only might break themselves when you use the one feature they provide. Inline Form Errors breaks all the forms on your site.
I'd encourage us to not automatically follow a standard at the expense of giving needed information. If this text is being removed from the module description, it needs to be added to the warning you get when you enable the module or something in the same patch.
However, I'd still prefer to leave this as is. The text should be scary. It is meant to be.
Comment #22
xjmRemoving the novice tag, since the path forward does not have consensus. Thanks for looking at this at DrupalCon.
(Edit: To clarify, this should not have been marked RTBC, since the RTBCed patch is exactly the same as the one catch marked "Needs work" earlier and does not incorporate feedback from him or @ifrik.)
Comment #23
xjmComment #24
xjmAlso, in general, we should take into account the reasoning behind previous changes when suggesting a new change. It always helps to look at why something ended up this way; our code does not come from nowhere. Adding the previous issues.
Comment #25
catchNote there's a contrib project already at https://www.drupal.org/project/ife
Comment #26
SKAUGHTThe contrib line of IFE (D7 only) is certainly not the same as what's been built into D8 core at this point. Keep in mind the IFE module is basically an 'on switch' 2578561 certainly, it is already deeply tied to the Form System in D8. To say remove IFE from core and use the standing conrtib module is impossible with whats in that repo.
is an overstatement. IMO, at this point the 'worst breaks' reported are
otherwise this tools is in good shape, certainly it doesn't 'break all forms'. This module is a great step forward for drupal, both in terms of general UX..let alone for accessibility (WTAG). It's deeply saddening that core isn't more committed to this initiative over small standing issues.
sorry for ranting here. otherwise i don't know where to post.
Comment #27
catchMigrate UI has a form, if that form has bugs in it, it only affects migrate UI.
The two bugs you mentioned affect any form with those kinds of elements - which is plenty of core forms to start with, even if it's not 'all forms'.
So the implications of breakage (however defined) are more severe for this module, which affects every form, vs. one that adds a standalone UI.
I opened #2729287: [policy, no patch] Decide if and how to remove experimental modules from core which is a better place for this part of the discussion.
Comment #28
sugaroverflow CreditAttribution: sugaroverflow commentedSince this is turning into part of a larger discussion, unassigning myself.
(Worked on this at Drupalcon and still a newbie to core!)
Comment #29
SKAUGHTDiscussion is everyday, it's part of how things happen. (: the sub-discussion has a place to happen now.
i'm re-running your patch as it was probably a test bot error.
thanks.
patch 14 does apply. was probably a test bot error. patch does complete original request of ticket.
Comment #30
ifrikSince I opened the issue, and it turns out to need more discussion, I set this to Needs work again. I see whether I can think of a different wording, that's shorter so that it does not break off.
Comment #31
ifrikComment #32
ifrikInstead of completely removing the second sentence I've shortened it into one sentence.
That way the information is less likely to break off, and is consistent with our help text standards.
(and I returned to the original patch name for consistency.)
Comment #33
ifrikComment #34
ifrikComment #35
ifrikComment #36
ifrikSorry, I had uploaded the wrong patch.
The new text now reads
Adds WCAG 2.0 accessibility compliance for web form errors, but some functionality might not work.
This should keep the scaryness of the description, but is shorter so that it does not break off on the Extend page.
Comment #37
catchThat seems better to me.
Comment #38
Gábor HojtsyLooks fine with me too.
Comment #40
Gábor HojtsyLooks like a random fail.
Comment #41
xjmAh, that's an excellent compromise! I think this is definitely an improvement that builds on the previous changes rather than reverting them.
Committed 3ed6e6b and pushed to 8.2.x. Thanks!
Comment #44
Gábor HojtsyThanks, removing from UX sprint now.