Skip to content

[4.0] Drop inline script for caption.js#15956

Merged
wilsonge merged 5 commits intojoomla:4.0-devfrom
dgrammatiko:#####4.0-no-inline-JS-03
Jul 3, 2017
Merged

[4.0] Drop inline script for caption.js#15956
wilsonge merged 5 commits intojoomla:4.0-devfrom
dgrammatiko:#####4.0-no-inline-JS-03

Conversation

@dgrammatiko
Copy link
Copy Markdown
Contributor

@dgrammatiko dgrammatiko commented May 10, 2017

Pull Request for Issue # .

Summary of Changes

  • Drop inline scripts
  • javascript tests fixes

Testing Instructions

if all tests green no tests needed!

Expected result

Actual result

Documentation Changes Required

@Bakual
Copy link
Copy Markdown
Contributor

Bakual commented May 11, 2017

Do we still need the caption.js? Imho all it does is what native HTML already can do for quite some time.

@laoneo
Copy link
Copy Markdown
Member

laoneo commented May 11, 2017

Would be nice to get rid of it.

@infograf768
Copy link
Copy Markdown
Member

as far as I remember, we kept it for B/C some time in the past.

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

There are 2 options here

  • create one more layout override in the front end template e.g. legacy.php
  • update all content to use figure (tinyMCE does that already, but probably needs an sql script for content table)

@brianteeman
Copy link
Copy Markdown
Contributor

update all content to use figure (tinyMCE does that already, but probably needs an sql script for content table)

I would be very reluctant to do anything that alters a users data

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

@brianteeman so we need one legacy.php override.
Anyways this PR mainly created as a guideline to the javascript tests for the GSOC student (no inline scripts and a way to cope with Joomla.storageOptions)

@Bakual
Copy link
Copy Markdown
Contributor

Bakual commented May 11, 2017

as far as I remember, we kept it for B/C some time in the past.

We kept it for B/C when we changed the image button to use the native elements. But with J4 we can imho drop it. Maybe make it available as extern plugin for those who still need it for some reason.

create one more layout override in the front end template e.g. legacy.php

What for? What would that override achieve?

update all content to use figure (tinyMCE does that already, but probably needs an sql script for content table)

Nah, never change user data. You can document it for users who want to do that themself or offer the JS as a plugin as I said above. But we need to drop that JS anyway and it will have the same impact if we do that in J4 or Jx.

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

@Bakual https://github.com/joomla/joomla-cms/pull/15956/files#diff-f6f264d0b0673dc25ecc75e7746ec235R77
it not even marked as deprecated ATM, so dropping it at least requires some prior notification...
Anyways I'm not against removing this legacy thing, just don't want to be the person to blame once again 😜

@mbabker
Copy link
Copy Markdown
Contributor

mbabker commented May 11, 2017

just don't want to be the person to blame once again 😜

You're the George of our JavaScript code, everything's your fault 🤣

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

I know 😞

@Bakual
Copy link
Copy Markdown
Contributor

Bakual commented May 11, 2017

it not even marked as deprecated ATM

We can easily deprecate it in 3.8.

@C-Lodder
Copy link
Copy Markdown
Member

I have tested this item ✅ successfully on 73222e9

code review


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

@zero-24
Copy link
Copy Markdown
Contributor

zero-24 commented May 29, 2017

@DGT41 can you take a look into the conflict?

@dgrammatiko
Copy link
Copy Markdown
Contributor Author

@zero-24 done!

@brianteeman brianteeman modified the milestone: Joomla 4.0 Jun 8, 2017
@wilsonge
Copy link
Copy Markdown
Contributor

wilsonge commented Jul 3, 2017

OK I'm going to merge this. But I've also created a follow up issue #16948 for dropping caption altogether as it's a non-trivial migration path so isn't 'straightforward'

@dgrammatiko dgrammatiko deleted the #####4.0-no-inline-JS-03 branch July 3, 2017 14:14
@joomla-cms-bot joomla-cms-bot changed the title [4.0] Drop inline script for caption.js [4.0] Drop inline script for caption.js Jul 3, 2017
@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.0 milestone Jul 3, 2017
@zero-24 zero-24 added this to the Joomla 4.0 milestone Jan 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants