Skip to content
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

Open
wants to merge 29 commits into
base: 5.2-dev
Choose a base branch
from

Conversation

stephan-ansems
Copy link

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 the showon attribute. However the effect is to toggle the required attribute on form fields and show/hode the little star next to the label that indicates that the field is required.
Code changes:

  • small refactoring of FormHelper method parseShowOnConditions to parseFieldConditions
  • new javascript asset requiron to perform the client-side magic
  • Addition of the requireon attribute and implementation in all places where the showon is also present

Why 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:

<?xml version="1.0" encoding="utf-8"?>
  <field
      name="foo"
      type="list"
      >
      <option value="1">MANDATORY</option>
      <option value="0">OPTIONAL</option>
  </field>
  <field
      name="bar"
      type="text"
      requireon="foo:1"
  />
</form>

Open the form and change the selection of the foo field and see the bar 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 the required 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 the showon 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.

@joomla-cms-bot joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.2-dev labels May 15, 2022
@stephan-ansems
Copy link
Author

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.

@richard67 richard67 changed the title mplement 'requireon' attribute to form fields for conditional requiredness Implement 'requireon' attribute to form fields for conditional requiredness May 15, 2022
@richard67
Copy link
Member

@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?

@dgrammatiko
Copy link
Contributor

@stephan-ansems out of curiosity: why didn't you extended the existing showOn functionality and duplicated the whole stack just for the extra attribute?

@rdeutz
Copy link
Contributor

rdeutz commented May 15, 2022

@stephan-ansems I think renaming that public method is a b/c break and so maybe not a good idea.

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

@stephan-ansems
Copy link
Author

@stephan-ansems out of curiosity: why didn't you extended the existing showOn functionality and duplicated the whole stack just for the extra attribute?

@dgrammatiko I don't see how i could do that. This seemed like the most obvious solution. Also I think that requireon and showon are not always linked, so hiding a field may influence the requiredness of it, but the reverse is not true.

@richard67
Copy link
Member

@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 stephan-ansems marked this pull request as ready for review May 16, 2022 17:04
@stephan-ansems stephan-ansems marked this pull request as draft May 16, 2022 18:33
@stephan-ansems stephan-ansems marked this pull request as ready for review May 16, 2022 19:33
@richard67
Copy link
Member

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

@joomla-bot
Copy link
Contributor

This pull requests has been automatically converted to the PSR-12 coding standard.

@HLeithner HLeithner removed the psr12 label Oct 23, 2022
@Hackwar Hackwar added the Feature label Apr 6, 2023
@HLeithner HLeithner changed the base branch from 4.2-dev to 5.0-dev May 2, 2023 16:42
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.0-dev.

@Hackwar Hackwar added the PBF Pizza, Bugs and Fun label Aug 25, 2023
@HLeithner HLeithner changed the base branch from 5.0-dev to 5.1-dev September 30, 2023 22:51
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.1-dev.

@HLeithner HLeithner changed the base branch from 5.1-dev to 5.2-dev April 24, 2024 09:09
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 5.2-dev.

@HLeithner HLeithner changed the title Implement 'requireon' attribute to form fields for conditional requiredness [5.2] Implement 'requireon' attribute to form fields for conditional requiredness Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature NPM Resource Changed This Pull Request can't be tested by Patchtester PBF Pizza, Bugs and Fun PR-5.0-dev PR-5.2-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants