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

[4.0] [com_associations] Improving display of the sidebyside page #27111

Merged
merged 4 commits into from
Nov 30, 2019

Conversation

infograf768
Copy link
Member

@infograf768 infograf768 commented Nov 21, 2019

See discussion #26945 (comment)

Summary of Changes

Moving the Target language field and buttons below the Target title preventing display of the language field with width to 100% until getting to mobile view.
This also aligns the item fields when using the same tab.
Correcting wrong js file name in the Association Field.
RTL css added

Testing Instructions

Create a multilingual site using the multilingual sample data module
Display the com_associations side by side page.
Reduce window to mobile size

Switch to RTL and check again

Before patch

sidebysidebefore

After patch

sidebysideafter

Important Note

Independent from this PR.
The Clear button is not acting as it correctly does in 3.x.
It should not only clear the existing association itself in the Reference but also reload the Target with empty item fields.
@dgrammatiko @Fedik

@Quy
Copy link
Contributor

Quy commented Nov 21, 2019

On phone, remove the margin under Reference.

@infograf768
Copy link
Member Author

If I remove it then the item fields will NOT be aligned.
See your own comment #26945 (comment)

@Quy
Copy link
Contributor

Quy commented Nov 22, 2019

On phone or small width, they won't ever be side by side.

27111

@infograf768
Copy link
Member Author

That part is easy to solve. Will do tomorrow.

@infograf768 infograf768 force-pushed the 4.0_sidebyside_display branch from 5596396 to e02b392 Compare November 23, 2019 07:05
@infograf768
Copy link
Member Author

@Quy Done
sidebysideafternew

@Fedik
Copy link
Member

Fedik commented Nov 24, 2019

I did not tested deeply, but I assume the problem with "change" event triggering (it does not triggers), that event is expected by associations-edit.js

if (formControlLanguage) {
formControlLanguage.addEventListener('change', (event) => {
// Remove message if any

The event should be triggered somewhere here:

} else if (task === 'undo-association') { // Undo association
var reference = document.getElementById('reference-association');
var target = document.getElementById('target-association');
var referenceId = reference.getAttribute('data-id');
var referenceLang = reference.getAttribute('data-language').replace(/-/,'_');
var targetId = target.getAttribute('data-id');
var targetLang = target.getAttribute('data-language').replace(/-/,'_');
reference = $(reference).contents();
target = $(target).contents();
// Remove it on the reference
// - For modal association selectors.
reference.find('#jform_associations_' + targetLang + '_id').val('');
reference.find('#jform_associations_' + targetLang + '_name').val('');
// - For chosen association selectors (menus).
reference.find('#jform_associations_' + targetLang).val('');
var lang = '';
// Remove it on the target
$('#jform_itemlanguage option').each(function()
{
lang = $(this).val().split('|')[0];
if (typeof lang !== 'undefined') {
lang = lang.replace(/-/,'_');
// - For modal association selectors.
target.find('#jform_associations_' + lang + '_id').val('');
// - For chosen association selectors (menus).
target.find('#jform_associations_' + lang).val('');
}
});
// Same as above but reference language is not in the selector
// - For modal association selectors.
target.find('#jform_associations_' + referenceLang + '_id').val('');
target.find('#jform_associations_' + referenceLang + '_name').val('');
// - For chosen association selectors (menus).
target.find('#jform_associations_' + referenceLang).val('');
// Reset switcher after removing association
var currentSwitcher = $('#jform_itemlanguage').val();
var currentLang = targetLang.replace(/_/,'-');
$('#jform_itemlanguage option[value=\"' + currentSwitcher + '\"]').val(currentLang + ':0:add');
$('#jform_itemlanguage').val('');
// Save one of the items to confirm action
Joomla.submitbutton('reference');

In Joomla 3 there an explicit call of .change(), that trigger change event by jQuery in sidebyside.js.
Since it still used jQuery there it should be possible to make/copy as it was in Joomla 3, except remove all .chosen() words.
The jQuery.change() will not work here.
In joomla 4 it can be triggered as
Joomla.Event.dispatch(element, 'change');
or
element.dispatchEvent(new CustomEvent('change', {bubbles: true, cancelable: true }));

@infograf768
Copy link
Member Author

@Fedik
My poor knowledge: I am unable to modify the sidebyside.js to fit your suggestion.

Example code:
3.x we have both a remove() and a change() line 48 on

		// - For chosen association selectors (menus).
			reference.find('#jform_associations_' + targetLang + '_chzn').remove();
			reference.find('#jform_associations_' + targetLang).val('').change().chosen();

in 4.0 line 41 on

      // - For chosen association selectors (menus).
      reference.find('#jform_associations_' + targetLang).val('');

and further down similar code

@Fedik
Copy link
Member

Fedik commented Nov 24, 2019

.... '_chzn').remove();

this creepy thing is for remove Chosen, just ignore it

reference.find('#jform_associations_' + targetLang).val('').change().chosen();

this to reset a value, and to trigger "change", hmm

I think something like:

var targetLangSelect = reference.find('#jform_associations_' + targetLang);
targetLangSelect.val('');
Joomla.Event.dispatch(targetLangSelect[0], 'change');

(Instead of what you seen in joomla 4 reference.find('#jform_associations_' + targetLang).val('');)

That a bit dirty, because a whole sidebyside.js is a hard mix of jQuery and non jQuery code, whoever made it.

@infograf768
Copy link
Member Author

@Fedik
alas I have now an error in core.js
TypeError: newElement is undefined core.js:1140:5

Line is
newElement.dispatchEvent(new CustomEvent(newName, {

@Fedik
Copy link
Member

Fedik commented Nov 25, 2019

hm, that sounds like targetLangSelect = reference.find('#jform_as... did not found anything

I will try to look more, on the week

@infograf768
Copy link
Member Author

will try to look more, on the week

Thanks. I guess that if you can solve, it would be better to create a new PR and ping me.

To help, this is what we get in 3.x when using the Clear button.

clear_assoc3x

The association is cleared from the reference.
The whole form is cleared in the Target. It will only display after selecting a Target Language.
As soon as a new Target item is selected, its name will display in the associations tab of the reference and the target associations tab will display the reference.

@infograf768
Copy link
Member Author

@Quy
As this work on sidebyside.js is totally independent from this PR, please test and let's try to get this one in. Thanks.

@Quy
Copy link
Contributor

Quy commented Nov 25, 2019

I have tested this item ✅ successfully on f92b2f1


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

@Fedik
Copy link
Member

Fedik commented Nov 26, 2019

@infograf768 I made a separated pull for that #27159

@infograf768
Copy link
Member Author

@Fedik
will test tomorrow

@richard67
Copy link
Member

@infograf768 In summary of changes section of description I think there is a typo:

... with width to 100% until getting to modal view.

I think you meant mobile view. Is that right?

@richard67
Copy link
Member

@infograf768 In a size short before mobile view - let me call it tablet view ;-) - it still does not look nice here:

side-by-side-1

It would look better if the left and right article would be aligned at the top. Not sure if that is possible, and not sure if what I see is correct. I mean I have done npm after applying the patch and have cleared broswer cache, but who kows, maybe that was not enough. Does it look the same for you with such a width?

Other question: I've noticed that since a few days when I install multilingual sample data, the English (GB) content is missing, only German and French content exists and is associated. Can you confirm that?

@infograf768 infograf768 force-pushed the 4.0_sidebyside_display branch from 5d011b5 to d0f20dd Compare November 29, 2019 07:23
@infograf768
Copy link
Member Author

@Quy @richard67
Modified to cope with tablet view

sidebyside_display

Please test again.

(Looking into the multingual sample data question, unrelated to this patch)

@infograf768
Copy link
Member Author

infograf768 commented Nov 29, 2019

@richard67
I confirm the issue with en-GB and multilingual sample data... GRRR
It is related to the absence of the lang string prefix for the xml as we have only
$file = $path . '/' . $lang . '/' . $lang . '.xml';
and not the possible situation
$file = $path . '/' . $lang . '/langmetadata.xml';
Looking into it now.

@infograf768
Copy link
Member Author

concerning sampledata, see #27179

@richard67
Copy link
Member

I have tested this item ✅ successfully on bb40f8c


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

1 similar comment
@Quy
Copy link
Contributor

Quy commented Nov 29, 2019

I have tested this item ✅ successfully on bb40f8c


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

@Quy
Copy link
Contributor

Quy commented Nov 29, 2019

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 29, 2019
@wilsonge wilsonge merged commit b7048c7 into joomla:4.0-dev Nov 30, 2019
@wilsonge
Copy link
Contributor

Thanks!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Nov 30, 2019
@wilsonge wilsonge added this to the Joomla 4.0 milestone Nov 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants