Skip to content

Comments

[3.10] Joomla.Showon.initialise API, and make sure that showon initialised only once, fork of 18843#32061

Merged
zero-24 merged 15 commits intojoomla:3.10-devfrom
Fedik:pull18843-2
Jan 22, 2021
Merged

[3.10] Joomla.Showon.initialise API, and make sure that showon initialised only once, fork of 18843#32061
zero-24 merged 15 commits intojoomla:3.10-devfrom
Fedik:pull18843-2

Conversation

@Fedik
Copy link
Member

@Fedik Fedik commented Jan 16, 2021

Pull Request for Issue #18843 and only for J3 .

Summary of Changes

This is fork of #18843, with requested changes.
The code make sure that showon initialised only once, no mater how often Joomla.setUpShowon was called.
This is achieved with extra flag showonInitialised that sets while showon initialisation.

Testing Instructions

Make sure showon still works.

Example, go to global config change "Debug Language" on/off, should downup/hides "Language Display" field.
Also the field "Language Display" will have extra attribute data-showon-initialised

Actual result BEFORE applying this Pull Request

showon works,
the showon field do not have data-showon-initialised

Expected result AFTER applying this Pull Request

showon works,
the showon field do have data-showon-initialised attribute

@richard67
Copy link
Member

@Fedik #18843 can be closed, right?

@Fedik
Copy link
Member Author

Fedik commented Jan 16, 2021

yeah, I have included changes from that pull

@dgrammatiko
Copy link
Contributor

@Fedik diffing the changes with the v4 version of show on I think you need to rename this to Joomla.Showon.initilise to be called like Joomla.Showon.initilise(field). The initialise in J4 could be a function in the Class with this code (the refactoring should be straight forward):

const jsondata = field.getAttribute('data-showon') || '';
const showonData = JSON.parse(jsondata);
let localFields;
if (showonData.length) {
localFields = [].slice.call(self.container.querySelectorAll(`[name="${showonData[0].field}"], [name="${showonData[0].field}[]"]`));
if (!this.fields[showonData[0].field]) {
this.fields[showonData[0].field] = {
origin: [],
targets: [],
};
}
// Add trigger elements
localFields.forEach((cField) => {
if (this.fields[showonData[0].field].origin.indexOf(cField) === -1) {
this.fields[showonData[0].field].origin.push(cField);
}
});
// Add target elements
this.fields[showonData[0].field].targets.push(field);
// Data showon can have multiple values
if (showonData.length > 1) {
showonData.forEach((value, index) => {
if (index === 0) {
return;
}
localFields = [].slice.call(self.container.querySelectorAll(`[name="${value.field}"], [name="${value.field}[]"]`));
if (!this.fields[showonData[0].field]) {
this.fields[showonData[0].field] = {
origin: [],
targets: [],
};
}
// Add trigger elements
localFields.forEach((cField) => {
if (this.fields[showonData[0].field].origin.indexOf(cField) === -1) {
this.fields[showonData[0].field].origin.push(cField);
}
});
// Add target elements
if (this.fields[showonData[0].field].targets.indexOf(field) === -1) {
this.fields[showonData[0].field].targets.push(field);
}
});
}

@Fedik
Copy link
Member Author

Fedik commented Jan 16, 2021

This only for J3, it should not go to J4,
there already event stuff for such thing:

/**
* Initialize 'showon' feature when part of the page was updated
*/
document.addEventListener('joomla:updated', ({ target }) => {

Only missed check for "double initialisation", I can do it later in another pull.

I do not think that this patch is much needed, but people asked 😉
It can be safely closed, depend what @HLeithner will decide

@HLeithner
Copy link
Member

@Fedik would be cool if we have the double initialization added in this pr too. After 2 tests we can merge it in to 3.10 (I think that makes more sense). I don't expect another 3.9 release (hopefully). It also introduces a new API endpoint so 3.10 is a better target.

@Fedik
Copy link
Member Author

Fedik commented Jan 17, 2021

would be cool if we have the double initialization added in this pr too

it is here, that why I made this fork:

// Set up only once
if ($target.data('showonInitialised')) {
return;
}
$target.data('showonInitialised', true);

I meant that for J4 will be need a separted PR.

It also introduces a new API endpoint so 3.10 is a better target.

I can remove Joomla.setUpShowon and leave "event" $(document).on('subform-row-add'), I guess most people who need it already use in this way, so it will be "bug fix" 😉

@Fedik Fedik changed the title Joomla.setUpShowon, fork of 18843 Joomla.Showon.initilise API, fork of 18843 Jan 17, 2021
@Fedik
Copy link
Member Author

Fedik commented Jan 17, 2021

Okay, so, I have backport Joomla 4 'joomla:updated' stuff for showon, now it can be more easy for transition.
And added Joomla.Showon.initilise as @dgrammatiko suggested, later I will make pull for J4.

Testing the same, make sure showon still work.

Please someone change target branch to joomla-10, or I have to make new PR?

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
@Fedik Fedik changed the title Joomla.Showon.initilise API, fork of 18843 Joomla.Showon.initilise API, and make sure that showon initialised only once, fork of 18843 Jan 17, 2021
@HLeithner HLeithner changed the base branch from staging to 3.10-dev January 17, 2021 10:18
@HLeithner
Copy link
Member

I change the branch for you.

But you can do this your self by adding the PR
image

Thanks for your PR, now we need some tests ;-)

@Fedik
Copy link
Member Author

Fedik commented Jan 17, 2021

well, I just broke PR
need some time to fix

okay, I think I need to sync with j 3.10 branch

@Fedik
Copy link
Member Author

Fedik commented Jan 17, 2021

@HLeithner can I merge joomla:3.10-dev to my branch for sync? or I need make new

@Fedik Fedik changed the title Joomla.Showon.initilise API, and make sure that showon initialised only once, fork of 18843 [3.10] Joomla.Showon.initilise API, and make sure that showon initialised only once, fork of 18843 Jan 17, 2021
@richard67
Copy link
Member

Yes, that will also work.

@HLeithner
Copy link
Member

I will upmerge staging

@HLeithner
Copy link
Member

so now it's ok again

@richard67
Copy link
Member

Yes, looks ok now.

@Fedik
Copy link
Member Author

Fedik commented Jan 17, 2021

I have updated testing instruction, and made same changes for joomla 4 #32069

@Fedik Fedik changed the title [3.10] Joomla.Showon.initilise API, and make sure that showon initialised only once, fork of 18843 [3.10] Joomla.Showon.initialise API, and make sure that showon initialised only once, fork of 18843 Jan 17, 2021
@dgrammatiko
Copy link
Contributor

I have tested this item ✅ successfully on 04e2d9d


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

1 similar comment
@ghost
Copy link

ghost commented Jan 22, 2021

I have tested this item ✅ successfully on 04e2d9d


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

@richard67 richard67 changed the title [3.10] Joomla.Showon.initialise API, and make sure that showon initialised only once, fork of 18843 [3.10] Joomla.Showon.initialise API, and make sure that showon initialised only once, fork of 18843 Jan 22, 2021
@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 22, 2021
@richard67 richard67 added RTC This Pull Request is Ready To Commit and removed RTC This Pull Request is Ready To Commit PR-staging labels Jan 22, 2021
@zero-24 zero-24 added this to the Joomla 3.10.0 milestone Jan 22, 2021
@zero-24
Copy link
Contributor

zero-24 commented Jan 22, 2021

@Fedik do we need to add documentation for this change somewhere? And if so what should be documented?

@Fedik
Copy link
Member Author

Fedik commented Jan 22, 2021

@zero-24 good question, I have no idea ;)
Do we have any documentation about showon API somwhere? if no, then maybe we need to add.

Technically and partly it is a new feature, to cover edge cases from #18843 , also see comment #18843 (comment)

In J4 we already have events #16016 (I backported it partly here), that handle "dynamic" content changes

@zero-24
Copy link
Contributor

zero-24 commented Jan 22, 2021

Ok will merge than this heree for 3.10. thanks

@zero-24 zero-24 merged commit a834f63 into joomla:3.10-dev Jan 22, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 22, 2021
@Fedik Fedik deleted the pull18843-2 branch January 22, 2021 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants