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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ifrik created an issue. See original summary.

ifrik’s picture

Status: Active » Needs review
FileSize
73.29 KB

I'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.

ifrik’s picture

The previous patch included composer file changes.

Status: Needs review » Needs work

The last submitted patch, 3: 2702545-inline-form-error-module-description-3.patch, failed testing.

NitinSP’s picture

Assigned: Unassigned » NitinSP
NitinSP’s picture

Assigned: NitinSP » Unassigned
Status: Needs work » Needs review
FileSize
1 KB

I have remove second line from description of .info file.Please review the latest patch #6.

Sonal.Sangale’s picture

Assigned: Unassigned » Sonal.Sangale
Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Needs review

This 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.

ifrik’s picture

NitinSP,

your patch includes the exact same text change as I've already done in my patch #3.

Why are you doing this?

ifrik’s picture

Thanks 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.

NitinSP’s picture

@ifrik

As per comment #4 your patch is failed and this issue is unassigned, so that I created that patch.

ifrik’s picture

NitinSP,

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.

sugaroverflow’s picture

I working on this at Drupalcon New Orleans.

sugaroverflow’s picture

aburrows’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
50.99 KB

Tested this and patch applied successfully. Awaiting test then RTBC
Mentored sugaroverflow on this issue / patch.

YesCT’s picture

Issue summary: View changes

The screenshot should probably be of the module page. Before and after would be nice.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: edit_the_inline_form-2702545-14.patch, failed testing.

snehi’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
29.14 KB
21.72 KB

Done the testing working fine.
Adding screenshots.

Before the patch:

Before

After the Patch

after

aburrows’s picture

Sweet, tested again locally and its working fine for me RTBC++

aburrows’s picture

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @ifrik and sprinters.

Other experimental modules are also listed as "alpha" on https://www.drupal.org/core/experimental but don't have this additional warning.

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.

xjm’s picture

Issue tags: -Novice

Removing 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.)

xjm’s picture

xjm’s picture

Also, 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.

catch’s picture

Note there's a contrib project already at https://www.drupal.org/project/ife

SKAUGHT’s picture

The 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.

Inline Form Errors breaks all the forms on your site.

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.

catch’s picture

otherwise this tools is in good shape, certainly it doesn't 'break all forms'.

Migrate 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.

sugaroverflow’s picture

Assigned: sugaroverflow » Unassigned

Since this is turning into part of a larger discussion, unassigning myself.
(Worked on this at Drupalcon and still a newbie to core!)

SKAUGHT’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Discussion 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.
ifrik’s picture

Since 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.

ifrik’s picture

Assigned: Unassigned » ifrik
Status: Reviewed & tested by the community » Needs work
ifrik’s picture

Assigned: ifrik » Unassigned
Status: Needs work » Needs review
Issue tags: +frontendunited
FileSize
632 bytes
603 bytes

Instead 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.)

ifrik’s picture

Issue tags: +Usability
ifrik’s picture

Issue tags: +DevDaysMilan
ifrik’s picture

Status: Needs review » Needs work
Issue tags: -frontendunited +sprint, +String change in 8.2.0
ifrik’s picture

Status: Needs work » Needs review
FileSize
671 bytes

Sorry, 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.

catch’s picture

That seems better to me.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine with me too.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 36: 2702545-inline-form-error-description-32.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community
testFieldUpdate8002
fail: [Completion check] Line 69 of core/modules/field/src/Tests/Update/FieldUpdateTest.php:
The test did not complete due to a fatal error.

Looks like a random fail.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Ah, 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!

  • xjm committed 3ed6e6b on 8.2.x
    Issue #2702545 by ifrik, NitinSP, sugaroverflow, snehi, aburrows, catch...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks, removing from UX sprint now.