Allow to pass in boolean to preventEnterSubmit directive

[bjarnef]

Prerequisites

• I have added steps to test this contribution in the description below

Description

At the moment preventEnterSubmit is evaluating the value of the attribute to decide if the directive should be enabled.
However this wasn't used anywhere:
https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/src/common/directives/components/forms/prevententersubmit.directive.js#L10-L17

The preventDefault directive have a similar check and does use this:
https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/src/common/directives/components/forms/preventdefault.directive.js#L33

PR #8179 introduced an <umb-search-filter> component.
I have updated preventEnterSubmit to use this and thus view of <umb-search-filter> component can be simplified. It also seems the default input id prefix umb-check_ was a copy/paste, so I have changes this to umb-search-filter_ instead.

Furthermore if updated umbAutoFocus with a similar check, so it doesn't necessarily need to be a string value, but can be e.g. use a boolean true/false value.

[bjarnef]

@kjac maybe you always want to confirm this works as expected? 🥇

[nul800sebastiaan]

This one is too intricate for me to evaluate, I will need some help from @kjac, @nathanwoulfe or if neither can help then @nielslyngsoe. Any takers? 🤞

[kjac]

I'll give this one a spin in the morning as well 🙂

[kjac]

@bjarnef this seems a tad excessive.

You're absolutely right that preventEnterSubmit doesn't use the calculated value of it's enabled property for anything at all, so the changes in prevententersubmit.directive.js are spot on.

The same goes for the changes in umb-search-filter.html - no need for all that duplicate code, passing in prevent-enter-submit="vm.preventSubmitOnEnter" works perfectly once preventEnterSubmit actually does what it's supposed to.

However - the rest of the code changes are not necessary for this to work:

• The changes to umbsearchfilter.directive.js break backwards compatibility for usages with truthy values (0/1, 'true'/'false' etc.). The renaming of vm.inputId is fine though, but not strictly necessary.

• The changes to umbautofocus.directive.js seem to belong in another PR?

All in all I would drop the changes to umbsearchfilter.directive.js and umbautofocus.directive.js in order to keep this PR neat and clean.

[bjarnef]

@kjac not sure if we need to support both 0/1 and 'true'/'false'?
The docs has defined it as a boolean 👍

Umbraco-CMS/src/Umbraco.Web.UI.Client/src/common/directives/components/forms/umbsearchfilter.directive.js

Line 30 in ad29e62

@param {boolean} preventSubmitOnEnter Set the enter prevent directive or not

Regarding vm.inputId it could potentially have side-effects if you wanted to inject some custom styling or javascript (maybe replacing check icon? 😂🤷‍♂️) if the css/js was targeting and element id starting with umb_check_ (this would both afffect checkbox and mini search then).

Yeah, the changed in umbautofocus.directive.js could probably be in a separate PR, but it thought I would made the "enabled" check consistent while working at this one.

[kjac]

@bjarnef aye... backwards compatibility is super important, and too often something that is overlooked when doing clientside changes. I think it would be best to keep supporting the thruthy values as well, and yes - retain the current (albeit wrong) input ID too.

[bjarnef]

@kjac we can make fallback to ensure truthly values works as well (although the docs clearly mention it should be a boolean) and if developers were using 'true' or 1 it would also have been working.

Not sure about the input ID though. It is not used for something specific in core except making the id unique enough, so the browser doesn't complain about duplicate ID's.
I think this could be categorized a bit like Umbraco doesn't supporting packages pointing directly to specific views. I think this would be okay in a minor release otherwise we could never fix mistakes like this. Sometimes classes also changes on elements, we can't guard against developers doing some css or js target on an element with a specific class either, which in a future release has changed or been removed 🙂

[kjac]

@bjarnef fine by me; the ID fix isn't terribly important so let's not spend too much time on it. Long as the directive maintains its backwards compatibility (ID change or not).

[bjarnef]

@kjac it should now check for truthly value 👍

[kjac]

Thanks @bjarnef. Now all that remains is for you to revert the changes to umbautofocus.directive.js`, then I'll take the PR for a test drive.

[bjarnef]

@kjac I have reverted the changes in umbAutoFocus directive and submitted this in a separate PR #8799

[bjarnef]

@kjac did you had a change to take a second look at this PR?

[bjarnef]

@nathanwoulfe maybe you could take a look a this one to ensure it also works when passing in a boolean value and make the code cleaner.

[bjarnef]

@nielslyngsoe @madsrasmussen maybe this is something you guys want to review as it would would ensure we actually can enable/disable this 😅😁

[bjarnef]

@emma-hq can we pick this up since it is currently not possible to disable preventEnterSubmit passing in false 🥇

[emma-hq]

Hi @bjarnef - sure we'll make sure this gets some attention. Thanks for all the work you've done trying to ensure this gets seen 🙌

[emma-hq]

Hey @bjarnef

Are you able to resolve the conflicts? I'll merge once that's done.

Em

[bjarnef]

@emma-hq I have merged latest changes from v8/contrib and resolved the conflicts.
I also noticed some inconsistency regarding the additional cssClass applied.
https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/src/views/components/forms/umb-search-filter.html#L2

https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/src/views/components/forms/umb-search-filter.html#L23

Resolved