Skip to content

Comments

[4.0] Fix webcomponents not being included on the page#24075

Merged
zero-24 merged 1 commit intojoomla:4.0-devfrom
wilsonge:webcomponent-include-fix
Mar 3, 2019
Merged

[4.0] Fix webcomponents not being included on the page#24075
zero-24 merged 1 commit intojoomla:4.0-devfrom
wilsonge:webcomponent-include-fix

Conversation

@wilsonge
Copy link
Contributor

@wilsonge wilsonge commented Mar 3, 2019

A b/c break in #19972 has caused web components scripts to no longer be enqueued for use by core.js - this reworks the webcomponent jhtml call to be a bit cleaner and work around the b/c break

@wilsonge
Copy link
Contributor Author

wilsonge commented Mar 3, 2019

@dgrammatiko as you posted on the original PR

Edge case if someone used this code to actually replace the values in the storage but then again I don't think that many people are actually using or even understand the Joomla.storageOptions.

I think that’s exactly what we are doing with webcomponents. We have added a single element array and rely on it merging into the existing array elements. After that PR the last entry overwrites the one before it :/

@wilsonge
Copy link
Contributor Author

wilsonge commented Mar 3, 2019

Drone passes now so this is definitely the root cause (although not definitely the right fix)

@wilsonge
Copy link
Contributor Author

wilsonge commented Mar 3, 2019

@SniperSister @zero-24 can you look into drone here so that it’s passing rips when everything here is fine please?

@dgrammatiko
Copy link
Contributor

@wilsonge changing it to array_merge_recursive fixes this?

@Hackwar
Copy link
Member

Hackwar commented Mar 3, 2019

My way to fix this was going to libraries/src/Html/HtmlHelper.php line 778 and exchanging 'js' with 'webcomponents/js'

@dgrammatiko
Copy link
Contributor

@Hackwar I don't think there is a webcomponents folder anymore...

@Hackwar
Copy link
Member

Hackwar commented Mar 3, 2019

I ran composer update and npm clean-install several times...

@Hackwar
Copy link
Member

Hackwar commented Mar 3, 2019

Ok, if we don't have a webcomponents folder anymore, as seems to be the case, then this here is wrong:

HTMLHelper::_('webcomponent', 'system/joomla-toolbar-button.min.js', ['version' => 'auto', 'relative' => true]);

@Hackwar
Copy link
Member

Hackwar commented Mar 3, 2019

Or rather this here:

HTMLHelper::_('webcomponent', 'system/fields/joomla-toolbar-button.min.js', ['relative' => true, 'version' => 'auto', 'detectDebug' => true]);

@dgrammatiko
Copy link
Contributor

@Hackwar nope, that's correct:
HTMLHelper::_('webcomponent', 'system/joomla-toolbar-button.min.js', ['version' => 'auto', 'relative' => true]);

Break down:

  • 'webcomponent' calls the fn webcomponent
  • 'system/joomla-toolbar-button.min.js' The path
  • 'relative' => true] The path is relative eg real path: system/js/joomla-toolbar-button.min.js
  • 'version' => 'auto' Version string applied, eg: ?fdkjhkldjfhk

@Hackwar
Copy link
Member

Hackwar commented Mar 3, 2019

I understand that, but these 2 calls reference the same file, while one exists and the other not. see dec68e1#diff-169f9020e1f708bc04e7ca0605989ac5R15

@dgrammatiko
Copy link
Contributor

@Hackwar yeah that's a bug, you can blame me for that. In sort the toolbar is not a field thus the script shouldn't live in that folder

@wilsonge
Copy link
Contributor Author

wilsonge commented Mar 3, 2019

@Hackwar I fixed that one already #24074

@wilsonge
Copy link
Contributor Author

wilsonge commented Mar 3, 2019

The problem is, is where and how we want to fix the “break” given this was merged and released in August in 3.8. We can change the function here back/to array recursive merge OR we can change web components understanding the value will get overwritten. Tempted for the latter to reduce b/c breaks?

@bembelimen
Copy link
Contributor

bembelimen commented Mar 3, 2019

Tempted for the latter to reduce b/c breaks?

Yes please! This fix wasn't without reason ;)

@roland-d
Copy link
Contributor

roland-d commented Mar 3, 2019

@wilsonge Tested this successfully. Current 4.0-dev the toolbar buttons are broken. This fix makes it work again.

@dgrammatiko
Copy link
Contributor

This fix wasn't without reason

If the only use case is tinyMCE then probably that's not a big problem as I see J4 coming out with the new tinyMCE which of course will break B/C with the old crappy way of initialising the editor (in sort will not use scriptOptions)

@wilsonge wilsonge force-pushed the webcomponent-include-fix branch from 896efbb to 985e196 Compare March 3, 2019 18:57
@wilsonge
Copy link
Contributor Author

wilsonge commented Mar 3, 2019

Reworked the webcomponent part and cleaned it up at the same time. Please test this now. In retrospect I really wish we'd done a recursive merge instead of recursive replace. But unfortunately the ship has sailed - and given it's easy to work around this for webcomponents here - I'm not going to try and introduce an unncessary b/c break again

@wilsonge wilsonge marked this pull request as ready for review March 3, 2019 18:58
@wilsonge wilsonge force-pushed the webcomponent-include-fix branch from 985e196 to db78ac2 Compare March 3, 2019 19:07
@Hackwar
Copy link
Member

Hackwar commented Mar 3, 2019

I have tested this item ✅ successfully on db78ac2


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

@roland-d
Copy link
Contributor

roland-d commented Mar 3, 2019

I have tested this item ✅ successfully on db78ac2

Buttons working again after applying PR


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

@zero-24 zero-24 added this to the Joomla 4.0 milestone Mar 3, 2019
@zero-24 zero-24 merged commit 9a80848 into joomla:4.0-dev Mar 3, 2019
@zero-24
Copy link
Contributor

zero-24 commented Mar 3, 2019

Merged thanks.

@wilsonge
Copy link
Contributor Author

wilsonge commented Mar 3, 2019

Thanks!

@wilsonge wilsonge deleted the webcomponent-include-fix branch March 3, 2019 23:28
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