Skip to content

Comments

Do not force jQuery and Bootstrap.js in Cassiopeia#23744

Closed
dgrammatiko wants to merge 2 commits intojoomla:4.0-devfrom
dgrammatiko:4.0-dev-cassiopeia
Closed

Do not force jQuery and Bootstrap.js in Cassiopeia#23744
dgrammatiko wants to merge 2 commits intojoomla:4.0-devfrom
dgrammatiko:4.0-dev-cassiopeia

Conversation

@dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Feb 2, 2019

Pull Request for Issue # .

Summary of Changes

  • Removed the dictated jQuery.js and Bootstrap.js from theCassiopeia template.
  • Reason: almost ALL interactive components for Bootstrap are held by a PHP helper (HTMLHelper::_('bootstrap.something'...)) therefor there is zero benefit forcing these scripts in the index.php of the template. Also we've (as the Javascript team) put a huge amount of work to convert everything to vanilla javascript and quite frankly by forcing jQuery always makes all that work redundant.

Testing Instructions

Check that everything still work as before in the front end

Expected result

Actual result

Documentation Changes Required

No. jQuery and Bootstrap.js are still supported and can be loaded whenever they're needed but they're not forced anymore

@dgrammatiko
Copy link
Contributor Author

With forced jQuery and bootstrap.js:
screenshot 2019-02-02 at 14 00 21

Without:
screenshot 2019-02-02 at 13 58 40

That .5s in my localhost translates to some seconds in real world with network latencies and other bottlenecks. Don't stream more javascript to the client than what is absolutely necessary...

@wilsonge
Copy link
Contributor

wilsonge commented Feb 2, 2019

I'd love it if this worked - unfortunately the menu doesn't work anymore after this is applied :(

@dgrammatiko
Copy link
Contributor Author

@wilsonge fixed

@brianteeman
Copy link
Contributor

Removed the dictated jQuery.js and Bootstrap.js from the backend template.

cassiopeia is fronted. i assume this was a typo or am i missing something else

@dgrammatiko
Copy link
Contributor Author

@brianteeman typo. Copy pasted the text from the other PR..

@brianteeman
Copy link
Contributor

thanks for confirming

@wilsonge
Copy link
Contributor

wilsonge commented Feb 2, 2019

@dgrammatiko I don't mean the mod_menu unfortunately - I mean the one in the template here https://github.com/joomla/joomla-cms/blob/4.0-dev/templates/cassiopeia/index.php#L90-L102

@dgrammatiko
Copy link
Contributor Author

@wilsonge I'm afraid that this is more of a problem with how the template is created. That whole html markup belongs to the menu module, splitting this to 2 parts one in the template and another in the module is wrong. The menu is one entity that includes also the button for the narrow screens. In sort, moving the bootstrap call into the menu module will solve the problem for mobiles but also unveiled a huge perception problem here: you cannot split things up just because you might be able to or you'll breaking up massively the modularity of the system.
Another example that this design of the template will fall really sort: I'm a user and have just installed a 3rd pt menu system. There are 2 options here either that module has to follow the given hardcoded button markup that already exists in the template or you'll stack with an extra button...
That's not promoting modularity or flexibility

My 2c anyways...

@wilsonge
Copy link
Contributor

Well actually it doesn't because you're saying you can't add things like search area's into the menu dropdown (which is a real user case).

I'm not against this PR but until we find a way to make that work this PR is not going to be merged

@dgrammatiko
Copy link
Contributor Author

because you're saying you can't add things like search area's into the menu dropdown

That's a wrong assumption, you can echo any module position inside any module template, so nothing is broken, just some minor shifting to get more modular...

@wilsonge
Copy link
Contributor

wilsonge commented Feb 22, 2019

It’s not an assumption. Right now we have a UI using module positions that is designed to do things just like what I said

This PR is totally legit in intentions. But right now we have a hard bootstrap dependency to give a specific user feature and we can’t merge this until we come up with a solution to that

@dgrammatiko
Copy link
Contributor Author

we can’t merge this until we come up with a solution to that

I will come up with something the coming days

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.

4 participants