Fix namespacing problem

[dojutsu-user]

Every function is now under READTHEDOCS object.
Users have flexibility to override any of them. (Tested)

[ericholscher]

This approach seems reasonable. @agjohnson & @davidfischer, do y'all have thoughts on this approach to namespacing and overrides? We should likely standardize on something like this on our internal JS everywhere, so this can be a good test if it looks good.

[dojutsu-user]

@ericholscher
I have updated the minified files.
I think we can upload it to pypi after this PR.

[davidfischer]

I favor namespacing everything under READTHEDOCS. However, I think we should try to have at least that variable defined before users override it. Otherwise, they might override everything we put in there which is probably not what we want.

[ericholscher]

@davidfischer can you expand a bit more on this? Is there a change to this PR needed?

[davidfischer]

This screenshot illustrates what I'm talking about. If we suggest people define a new READTHEDOCS object:

var READTHEDOCS = {
    getInputField: function() {
        var custom_search_bar = document.querySelector('.my-custom-search-bar');
        return custom_search_bar;    
    }
}

Rather than if the READTHEDOCS object was guaranteed to be present already and they only override the one part they want:

READTHEDOCS.getInputField: function() {
    var custom_search_bar = document.querySelector('.my-custom-search-bar');
    return custom_search_bar;    
}

This could open up a number of issues based on the order JS files are included.

[davidfischer]

Other than the comment above, I do want you to think about which functions you want users to override and which ones you don't. Only functions users might override should be on the READTHEDOCS object. Functions like getInputField() or fetchAndGenerateResults() are ones that users or themes would definitely want to override. However, functions like createDomNode() seem private/protected to search and not something the user would override.

[ericholscher]

@dojutsu-user Any update here?

[dojutsu-user]

@ericholscher
Nothing here.
I will make all the PRs ready to be merged in master so that they can go out this week.

[dojutsu-user]

I will fix the conflicts and make this mergeable after merging of #42

[dojutsu-user]

@davidfischer
I have updated the PR to not include the private methods.
but I'm not sure what should be the correct approach for your comment above.

Do you have anything in mind?
I do tested this thing and I think that for normal use cases, order of the js files should be correct.

[ericholscher]

@stsewd have you looked at this at all?

[stsewd]

I'm not sure to understand the use case for this.
Also, currently a lot of these functions are in the global scope (and also variables), so they could easily collide with other functions defined by the user or other extensions.

For the example in the docs, allowing to override getInputField to have a custom search input, I think is better to just force the users to make use of the ARIA role [role='search'] input so we can capture it (we can document this).

For other customizations, I think we should provide classes/ids and document them so users can modify them using selectors in js/css. If we have a use case that can't be extended using that, I think we can consider allowing users to override some functionality.

[ericholscher]

Also, currently a lot of these functions are in the global scope (and also variables), so they could easily collide with other functions defined by the user or other extensions.

This is exactly the issue. The goal is to nest all the code under the READTHEDOCS namespace to avoid collisions and make it easier to extend.

[ericholscher]

@humitos Is this solved by our new module approach, I assume?

[humitos]

@humitos Is this solved by our new module approach, I assume?

In the new module, we don't have a global object. All the configs come from the API and cannot be edited, so this is not an issue there. We haven't defined the user API to interact with our extension yet. I have an issue opened to talk about these things and use the best/standard js-y approach we can: readthedocs/addons#13

I would appreciate feedback and ideas there 😄