-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[5.2] Implement 'requireon' attribute to form fields for conditional requiredness #37798
base: 5.2-dev
Are you sure you want to change the base?
Conversation
This implements the toggling of requiredness in a similar fashon like the showon assed does with visibility
Renamed method and variables to make it less about showOn conditions and more about field conditions so it can be reused by requireon without cringing...
To any of the experienced Joomla developers, i'm learning how to do this and I appologize for things that are not according to guidelines or procedures. Please help me learn them. |
@stephan-ansems I think renaming that public method is a b/c break and so maybe not a good idea. It also seems to break the installation form, at least the automated test break when making a new installation. Have you tested that, make a new Joomla installation with your changed code? Or in an existing installation, change something in Global Configuration in backend and try to save the changes? |
@stephan-ansems out of curiosity: why didn't you extended the existing showOn functionality and duplicated the whole stack just for the extra attribute? |
@richard67 is right, renaming makes sense but you need to add the old function calling the new function. The old function should be depreciated 5.0. That should fix the testing problem for now. But we need to change the tests and replace all the calls to the old function. Good addition btw. |
@dgrammatiko I don't see how i could do that. This seemed like the most obvious solution. Also I think that |
Is it realy that important?
@stephan-ansems If you think the PR is ready for being tested, it needs you to click the "Ready for review" button at the bottom of the PR on GitHub to remove the "Draft" status. |
Thanks to @richard67 Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@stephan-ansems It does not need to update your branch every time GitHub shows that the branch is not up to date. It needs to do that only when GitHub shows in addition that there are conflicting files, which is not the case here. |
This pull requests has been automatically converted to the PSR-12 coding standard. |
This pull request has been automatically rebased to 5.0-dev. |
This pull request has been automatically rebased to 5.1-dev. |
This pull request has been automatically rebased to 5.2-dev. |
Pull Request for a new Feature
Summary of Changes
Implementation for an new attribute
requireon
to the form field spec to that accepts conditional statements identical to that of theshowon
attribute. However the effect is to toggle therequired
attribute on form fields and show/hode the little star next to the label that indicates that the field is required.Code changes:
parseShowOnConditions
toparseFieldConditions
requiron
to perform the client-side magicrequireon
attribute and implementation in all places where theshowon
is also presentWhy is this useful?
Some argue that having to toggle the requiredness of a field is a design flaw in the form, however I believe that it is not. For example a form to specify OAuth2 settings could have a dropdown to indicate the grant type. Depending on the grant type chosen, the form can change quite drastically. Some fields are not relevant for certain grant types and can be hidden, others are no longer required. The problem with only
showon
is that you can hide these fields, but a hidden field still remains required and form validation will not pass succesfully.Testing Instructions
Create a form with a listfield with two options and a field that has the new attribute, for example this content:
Open the form and change the selection of the
foo
field and see thebar
field become mandory or optional.Actual result BEFORE applying this Pull Request
Any form field that uses the
requireon
attribute remain required or optional depending on value of therequired
attribute. Joomla ignores the extra attribute, but adding the attribute to fields before this feature is implemented makes no sense.Expected result AFTER applying this Pull Request
Any form fields that have the
requireon
attribute will be conditionally required when the condition evaluates to true. The logic works the same as for theshowon
attribute.Documentation Changes Required
(https://docs.joomla.org/Form_field#Modal_form_field_types) can probably be documented in one fell swoop with the ShowOn property explanations.