NPM Resource Changed PR-5.0-dev Pending

User tests: Successful: Unsuccessful:

avatar GeraintEdwards
GeraintEdwards
16 Jun 2023

Pull Request for Issue # .

Summary of Changes

Place all the functions from guidedtours.es6.js into a an object "Joomla.guidedTour" so instead of calling getTourInstance() we would now call Joomla.guidedTour.getTourInstance() instead.

This will help avoid function name clashes, and is a step towards making the code more extensible.

Testing Instructions

Run the tours included in Joomla 5

Actual result BEFORE applying this Pull Request

The tours run as expected

Expected result AFTER applying this Pull Request

The tours run as expected

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

avatar joomla-cms-bot joomla-cms-bot - change - 16 Jun 2023
Category JavaScript Repository NPM Change
avatar GeraintEdwards GeraintEdwards - open - 16 Jun 2023
avatar GeraintEdwards GeraintEdwards - change - 16 Jun 2023
Status New Pending
avatar GeraintEdwards GeraintEdwards - change - 16 Jun 2023
Title
Namespace guided tour functions by wrapping in Joomla.guidedTour object
[5.0] Namespace guided tour functions by wrapping in Joomla.guidedTour object
avatar GeraintEdwards GeraintEdwards - edited - 16 Jun 2023
avatar dgrammatiko
dgrammatiko - comment - 16 Jun 2023

The first line of the script import Shepherd from 'shepherd.js'; indicates that this is an ES Module script. On top of that the script is loaded AS module:

Therefore there is NO need for namespacing as the whole file has it's own scope: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#other_differences_between_modules_and_standard_scripts so this PR is undoing any modern practices and falls the script to the jQuery era.
Please close it, you're not improving anything...

avatar laoneo
laoneo - comment - 17 Jun 2023

I do agree with the arguments of @dgrammatiko. So @GeraintEdwards what is your use case? Why do you need to make it extensible? Perhaps we need to find a way that extensions can interact with the functionality of guided tours which is ES module compatible.

avatar GeraintEdwards
GeraintEdwards - comment - 18 Jun 2023

@dgrammatiko I have been looking at this javascript mainly in the webbrowser to figure out how it was working and it was loading the es5 version as 'nomodule' each time I checked (latest version of Firefox in Linux). The really bizarre thing is that at some point this morning when I was rebuilding the scripts it switched to 'module' and I then tried and tried to recreate the nomodule behaviour but couldn't.

I was just about to post this response and I see it again
<script src="/media/plg_system_guidedtours/js/guidedtours-es5.min.js?b1c9af93e530977e63de06083aca3fea" nomodule="" defer=""></script>

Something very strange going on :(

Nevertheless I would argue that these methods need to be exported/made available in a global object to allow them to be interacted with.

@laoneo I can see a few scenarios where we may wish to extend or modify the behaviour of these methods.

  1. In my case I wish to build training tools for my extensions which go well beyond the functionality of the existing guided tours methods and I don't foresee an appetitive to incorporate all of this into the core guided tours code. Examples would be pre-step javascript execution or css/dom manipulation so multiple1 elements on the page could be highlighted, conditional steps based on the outcomes, promises/waiting dependencies so that after step triggered javascript has executed we can provide a message/move to the next step
  2. I think we should allow interaction with specific element types to be customised - I already have a PR ready to go that allows us to specify an input is required (when its not required in the DOM) e.g. for tour relating to backend filtering (where the search input box is not required) but the tour needs this to be required. That specific example is an functional oversight but its possible someone may want, for example, to create a tour with special handling of certain text fields where the input value is entered automatically and highlighted so the user doesn't actually type the values themselves.

My idea is to possibly add 'awaihandler', 'stephandler', 'prehandler', 'posthandler', 'choicehandler' type capability so that when steps and tours are loaded/navigated that these methods (if set for the relevant step) are called by the script.

Additionally the code is peppered with switch and "if/else if" statements based on element types or step types . It would be good, in my mind, to make this more object oriented.

At present its not possible to customise the handling of certain methods let alone implement custom step types or element handling.

I don't think the core should have a really complicated guided tour system by default but it should make it possible to allow 3PD or training providers to implement more advanced tours if they wish.

avatar dgrammatiko
dgrammatiko - comment - 18 Jun 2023

Something very strange going on :(

nothing strange happening, Joomla renders both scripts (with type=module and nomodule) but any browser >2018 will use only the type=module

Nevertheless I would argue that these methods need to be exported/made available in a global object to allow them to be interacted with.

actually the script itself does not expose anything by design. There’s no api to extend anything. If you want to customize this script Joomla already provides a way: use an override and write your own take. About global objects: that’s not how we code anymore in 2023, neither in js nor in php…

avatar GeraintEdwards
GeraintEdwards - comment - 18 Jun 2023

@dgrammatiko

Thanks for your input.

Are you suggesting the best way to extend this functionality is to override the whole file via the WebAssetManager? TBH I've been stuck having to support 'dead' browsers for too long and not ever expecting to be able to use more advanced JS any time soon :( Sorry for the miss-understanding.

I can see how we we load our own version of the js in place of the preset assets but replacing the whole file feels like overkill. If the built in file was 'class based' we could 'inherit' the base methods or the existing functions were exported then an override script would only override the methods that need changing or could re-use the exported functions. Did I miss something else in how modules work?

What do think of my point about reducing the use of switch and "if/else if" statements?

avatar dgrammatiko
dgrammatiko - comment - 18 Jun 2023

TBH I've been stuck having to support 'dead' browsers for too long and not ever expecting to be able to use more advanced JS any time soon :( Sorry for the miss-understanding.

FYI Joomla 5.0 will NOT support old/dead browsers once #39618 is merged.

but replacing the whole file feels like overkill.

In this particular case it's not as the API for the guided tours was not designed to be extended. Also providing your own file you could form the API/functionality the way you want. So without the need the core script to be needless more complicated you could achieve what you want right now, thus I don't see how overengineering something (that won't provide anything to core apart that maintainers would have to maintain code that it won't be used anywhere in the core) is the way to go. Also overriding the whole file has another advantage: less code both in the server side and less scripts glued together on a global object, so probably a better design altogether...

if the built in file was 'class based' we could 'inherit' the base methods or the existing functions were exported then an override script would only override the methods that need changing or could re-use the exported functions

That's not how most of the JS should be extended (class inheritance). It's, mostly, preferred to be event based. Ie have the guided tours fire events and then depending on the step/input/whatever a specific event listener/handler will run. But, guided tours was neither coded as a class and neither have an event bus/system in place. So back to overriding the whole file is the preferred way to go. Also, really, even if the class inheritance is the preferred way here (I doubt it), the class SHOULD NOT be a global object for many reasons...

avatar HLeithner
HLeithner - comment - 18 Jun 2023

Having it not a global object make sense, but having no events doesn't make sense, especially if joomla's main strange is the event system (in php) and the extensibility. Maybe it's not needed to directly extend the guidedTour object, having the possibility to prepare things before or after a step is execute make sense for me.

avatar GeraintEdwards
GeraintEdwards - comment - 19 Jun 2023

I'll shelve this PR for now and have a think about events, breaking out the handling of different DOM element types etc. to make the code more sustainable/maintainable.

avatar GeraintEdwards GeraintEdwards - close - 19 Jun 2023
avatar GeraintEdwards GeraintEdwards - change - 19 Jun 2023
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2023-06-19 08:28:47
Closed_By GeraintEdwards
Labels Added: NPM Resource Changed PR-5.0-dev

Add a Comment

Login with GitHub to post a comment