? ? Pending

User tests: Successful: Unsuccessful:

avatar GeraintEdwards
GeraintEdwards
23 Jul 2017

Pull Request for Issue # None.

Summary of Changes

The showon attribute currently works for fields in Jforms to allow their display to be conditional on the value of another field. This PR makes some simple changes to enable this functionality to work on list option value too - in other works the display of a specific list option can now be dependent on the value of another field.

The changes required to make this work are minimal:

  1. In libraries/joomla/form/fields/list.php we add pass new attribute name as an option

$listoptions['option.attr'] = 'optionattr';

this means that the list renderer will use the option.attr value we pass in to add additional attributes to the option.

  1. In the getOptions method of libraries/joomla/form/fields/list.php we look for a 'showon' attribute and if its specified pass it as 'optionattr'

  2. In media/jui/js/cms-uncompressed.js/media/jui/js/cms.js we have to disable the normal animation effect for option values (this does work in Chrome but not in other browsers) and add some special handling to toggle the display of the option and trigger an update in the chosen display where needed).

So all in all minimal changes

Testing Instructions

Best test is to edit an existing form e.g. administrator/components/com_content/config.xml and look for the declaration of targetb and add a showon attribute e.g.

<option value="1" showon="targeta:1">JBROWSERTARGET_NEW</option>

This make the browser new option in the URL b target drop down dependent on the value of URL a target drop down.

Expected result

When you edit the articles options and set URL A target drop down equal to 'new browser window' this option is available for URL B but otherwise its hidden.

optionvisible

optionhiden

Actual result

As explained above.

Documentation Changes Required

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
5.00

avatar joomla-cms-bot joomla-cms-bot - change - 23 Jul 2017
Category Libraries JavaScript
avatar GeraintEdwards GeraintEdwards - open - 23 Jul 2017
avatar GeraintEdwards GeraintEdwards - change - 23 Jul 2017
Status New Pending
avatar GeraintEdwards GeraintEdwards - change - 23 Jul 2017
Labels Added: ?
avatar tonypartridge tonypartridge - test_item - 25 Jul 2017 - Tested successfully
avatar tonypartridge
tonypartridge - comment - 25 Jul 2017

I have tested this item successfully on 632ca6c

Working very well. Tested on a few components with different values.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17212.

avatar tonypartridge
tonypartridge - comment - 25 Jul 2017

@C-Lodder can you review the JS?


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17212.

avatar dgt41
dgt41 - comment - 25 Jul 2017

@C-Lodder we need to deprecate the cms.js in j3 and remove it in j4. The code in that file should be either a custom element or some plain js attached to the relative field, same approach as all the rest fields that have some sort of js functionality (calendar, colour, etc)

avatar GeraintEdwards
GeraintEdwards - comment - 25 Jul 2017

The showon elements of the cms.js file relates to an attribute that can be applied to most fields so isn't field specific so if JForm specific as opposed to any specific JField type.

The list option specific aspect could be extracted to a separate file but we would need a javascript 'architecture' that allowed for this using callback or pseudo class methods ???

I know this isn't the place to have the debate but if we are not going to use jQuery then we will need our own slimmed down javascript library that is unit tested to avoid duplication of coding effort and code that fails in specific scenarios.

avatar dgt41
dgt41 - comment - 25 Jul 2017

will need our own slimmed down javascript library

No. We have ES5 ES6 and web components. No need for anything else!

avatar GeraintEdwards
GeraintEdwards - comment - 25 Jul 2017

@dgt41
I presume you mean we would use code like

xpathresults = document.evaluate('//*[@name="' + fieldName + '"]|//*[@name="' + fieldName + '[]"]', document, null, XPathResult.UNORDERED_NODE_SNAPSHOT_TYPE, null)				

in J4.0 instead of

$fields   = $('[name="' + fieldName + '"], [name="' + fieldName + '[]"]');

That sort of change is quite straightforward for J4.0 and doesn't need a javascript library like jQuery but what about little methods that are used repeatedly in different contexts such as the toggle method discussed above which keeps track of the original display style value, or little animations that would apply to ? Are we not going to have this type of re-usable utility function in J4.0? Would these be added to HTMLElement in a namespace safe way (sorry not sure of the jargon here)?

avatar dgt41
dgt41 - comment - 25 Jul 2017

@GeraintEdwards modern UI should be done through web component or custom elements. If we do it correctly jQuery will be useless (in fact it's already), because frankly we shouldn't do any animations with js but rather pass that to css (60FPS, 16ms Budget and almost all the current recommendations fail with jQuery, especially if you do the test in a cheap android device).
Anyways what you're doing here is fine, but for J4 we might have to rethink the whole approach to these fields.

If you're interested about the custom elements, check https://github.com/joomla-projects/custom-elements
and the relative PR for J4: #17024

avatar joomla-cms-bot joomla-cms-bot - change - 4 Dec 2017
Category Libraries JavaScript Unit Tests Repository Administration com_admin SQL Postgresql MS SQL com_associations
avatar C-Lodder
C-Lodder - comment - 4 Dec 2017

Can you sort out the PR so that the only the relevant commits are added?

avatar GeraintEdwards
GeraintEdwards - comment - 4 Dec 2017

I think I merged and pushed the wrong branch and now I'm trying to figure it out how to reverse this

avatar GeraintEdwards GeraintEdwards - change - 4 Dec 2017
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 4 Dec 2017
Category Unit Tests Repository Administration com_admin SQL Postgresql MS SQL com_associations Libraries JavaScript
avatar GeraintEdwards GeraintEdwards - change - 4 Dec 2017
Labels Removed: ?
avatar GeraintEdwards
GeraintEdwards - comment - 4 Dec 2017

Sorted out the bad merge - now the compressed js is up to date with staging and the PR.

What do we need to do to move this forward?

The showon code needs a lot of work:

  1. It really slow using jQuery and a large form (I have a prototype version implemented using native query selectors with a pseudo polyfill that falls back to jQuery where the query selector methods don't exist)
  2. the toggle needs to be replaced with a CSS transition (this is a very small performance benefit compared to the gains available from improving the finding of conditional elements and processing them on large forms)
  3. the toggle/its replacement needs to respect the original CSS state - e.g. a div shouldn't be reset to block if it was an inline-block beforehand
  4. the xml syntax should allow for and/or to be combined intelligently with appropriate associative ordering/parenthesis support

A lot of this could be done before 4.0 and I propose putting a PR together to deal with it in the next couple of weeks.

In the meantime cane we move forward with this PR please?

avatar joomla-cms-bot joomla-cms-bot - change - 6 Dec 2017
Category Libraries JavaScript Unit Tests Repository Administration com_admin SQL Postgresql MS SQL com_associations
avatar GeraintEdwards
GeraintEdwards - comment - 6 Dec 2017

Sorry guys - same problem again with the merge commit. Fixing it now

avatar GeraintEdwards GeraintEdwards - change - 6 Dec 2017
Labels Added: ?
avatar GeraintEdwards
GeraintEdwards - comment - 6 Dec 2017

I'm closing this PR and opening a new one based on a new 'clean' branch.

I'll include references to this one.

avatar GeraintEdwards GeraintEdwards - change - 6 Dec 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-12-06 11:54:09
Closed_By GeraintEdwards
avatar GeraintEdwards GeraintEdwards - close - 6 Dec 2017
avatar C-Lodder
C-Lodder - comment - 6 Dec 2017

Before you submit a PR, I suggest you always update your fork so that you're in sync with the main repo.

Git:

$ git remote add upstream https://github.com/joomla/joomla-cms.git
$ git fetch upstream staging
$ git merge upstream/staging
$ git push origin staging

$ git checkout -b NEW_BRANCH_NAME
// Then do your code changes

Git for Windows/Mac:

  • No idea
avatar GeraintEdwards
GeraintEdwards - comment - 6 Dec 2017

Thanks - I was trying to merge upstream using my IDE. I'll do it on the command line next time !!

PR is now resurrected as #18998

Add a Comment

Login with GitHub to post a comment