? Success

User tests: Successful: Unsuccessful:

avatar dgt41
dgt41
25 Feb 2016

Something odd was there

This just reformats the code on the JLayouts and properly using the API for inline JS and CSS

Testing

Make sure that codemirror is still working as expected

avatar dgt41 dgt41 - open - 25 Feb 2016
avatar dgt41 dgt41 - change - 25 Feb 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 25 Feb 2016
Labels Added: ?
dc91c66 25 Feb 2016 avatar dgt41 CS
avatar wilsonge
wilsonge - comment - 25 Feb 2016

Travis please

avatar wilsonge
wilsonge - comment - 25 Feb 2016

@okonomiyaki3000 can you review this please as the codemirror expert here :)

@dgt41 what is the result here if someone has overriden these JLayouts in a template with close to the existing code?

avatar dgt41
dgt41 - comment - 25 Feb 2016

@wilsonge this will be introduced in 3.5, so there is no way someone ever used that Layout
#5863

c6915aa 26 Feb 2016 avatar dgt41 CS
de1d7eb 26 Feb 2016 avatar dgt41 CS
avatar dgt41
dgt41 - comment - 27 Feb 2016

I reverted the last changes, so JLayouts are still used here, although at some point we should introduce a way to do javascript and css customisations with another approach, maybe JAssets, similar to JLayouts but for static files?

avatar okonomiyaki3000
okonomiyaki3000 - comment - 27 Feb 2016

I see now that a problem with using layouts to generate js or css (or really to use them anywhere other than in a template or another layout file) is that they are able to generate some kind of debug message. That's really unfortunate and actually the way that debug message works is rather terrible, in my opinion. Obviously, we have to work with what we have. Personally, I'd still prefer to use layouts for all the benefits they offer and I can I propose a solution that still works when using the debug mode if anyone is interested.

avatar dgt41
dgt41 - comment - 27 Feb 2016

@okonomiyaki3000 if you can short out the debug problem then using JLayouts for js and css will be fine. It will be highly appreciated if you do a PR on that

avatar okonomiyaki3000
okonomiyaki3000 - comment - 27 Feb 2016

I'll do it. But I can't promise the solution won't be slightly controversial. :wink:

avatar dgt41
dgt41 - comment - 27 Feb 2016

Even if it requires some extra stuff appended in the layout files (or anything similar), I think, will be acceptable as it will expand the usage of JLayouts

avatar okonomiyaki3000
okonomiyaki3000 - comment - 27 Feb 2016

@dgt41 I sent you a pull request with my quick solution to the issue. I'll think more about how the issue with layouts may be improved more generally and put in a different PR if I come up with anything good.

7be8139 27 Feb 2016 avatar dgt41 CS
avatar wilsonge
wilsonge - comment - 29 Feb 2016

Where are we with this? Ideally I'd like to have this merged in asap

avatar wilsonge wilsonge - change - 29 Feb 2016
Labels Added: ?
avatar okonomiyaki3000
okonomiyaki3000 - comment - 29 Feb 2016

I'm OK with it. From what I can see there are two possible complaints but feel free to ignore them because you can never please everyone.

1) Doing the css and js as strings removes the ability of most editors to do syntax coloring which was part of the reason for putting them in layouts in the first place. (The more important reason was to make them overridable so as long as we have that, I'm fine.)

2) Layouts may be an inappropriate way to do js and css. I don't really see why as they provide an easy way for these things to be overridden and there is good reason to believe someone may want to override them. If this is truly and inappropriate use of layouts then it should be changed before there's any possibility of someone relying on them. Since I would really hate to lose the ability to override these things, maybe using JHtml could be a solution.

avatar wilsonge
wilsonge - comment - 2 Mar 2016

I mean layouts aren't an ideal place to do this - but at the end of the day as you say it seems ideal and I think by ensuring that adding html comments won't break the css and js being returned means that we have a good compromise for putting CSS and JS in a standalone file. We're loosing the ability for the syntax coloring which kinda sucks - but the alternative of returning CSS and JS strings has even more pitfalls I fear :( I'll merge this tomorrow morning

avatar okonomiyaki3000
okonomiyaki3000 - comment - 2 Mar 2016

On a side note, I really think something should be done about the debugging mode in JLayoutFile. Just echoing some code wrapped in a <pre> tag is not a good idea. Even if layouts were only ever used as they were apparently intended (from within template and layout files), this approach doesn't work all that well. For example, in a case that a layout file is rendered within another layout file (maybe in a loop to generate or list or table rows or something), wouldn't you end up with things showing up in a weird order? I think we need a better solution for this. Maybe use the debug console or something.

avatar wilsonge
wilsonge - comment - 2 Mar 2016

I agree - although it appears that <pre> tag has existed for a hell of a long time without anyone coming across it... I mean we need something so that users know where to go to find the layout being used - that's the issue....

avatar brianteeman
brianteeman - comment - 2 Mar 2016

Yeah I wasted a lot of time looking

avatar okonomiyaki3000
okonomiyaki3000 - comment - 2 Mar 2016

I think a comment would work better than a <pre> for the reason that comments are legal anywhere, <pre> is not. For example, if your layout generated a table row and was obviously meant to be used within a <table>, a <pre> is going to be invalid.

I also think it's not unreasonable to expect developers to pass some value in $options to specify what kind of output the layout is expected to generate. Then an appropriate comment syntax could be used. For even more control and future-proofing, allow them to pass in a format string for comments and just use that.

Another issue (correct me if I'm wrong) is that layout debugging can only be turned on in code. We should be able to switch this on in the administrator, right?

avatar wilsonge
wilsonge - comment - 2 Mar 2016

We did try for 3.5 (well it was HTML comments rather than /* foo */) and all bootstrap modals immediately fell apart so we had to revert it :( - because the BS modals in JHtml were using JLayouts in exactly the same way that codemirror is before this PR is applied - which is why I wanted it changed!

avatar wilsonge wilsonge - change - 2 Mar 2016
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2016-03-02 10:10:17
Closed_By wilsonge
avatar wilsonge wilsonge - close - 2 Mar 2016
avatar wilsonge wilsonge - close - 2 Mar 2016
avatar wilsonge wilsonge - close - 2 Mar 2016
avatar wilsonge wilsonge - change - 2 Mar 2016
Milestone Added:
avatar wilsonge wilsonge - change - 2 Mar 2016
Labels Removed: ?
avatar brianteeman brianteeman - change - 27 Mar 2016
Category Layout
avatar brianteeman brianteeman - change - 27 Mar 2016
Status New Pending
avatar brianteeman brianteeman - reopen - 27 Mar 2016
avatar brianteeman brianteeman - reopen - 27 Mar 2016
avatar joomla-cms-bot joomla-cms-bot - change - 27 Mar 2016
Milestone Removed:
avatar wilsonge
wilsonge - comment - 27 Mar 2016

@brianteeman Any reason why you reopened this?

avatar brianteeman
brianteeman - comment - 27 Mar 2016

It wasnt closed on the issue tracker ;)

avatar brianteeman brianteeman - change - 27 Mar 2016
Status Pending Closed
Closed_Date 2016-03-02 10:10:17 2016-03-27 11:25:41
Closed_By wilsonge brianteeman
avatar brianteeman brianteeman - close - 27 Mar 2016
avatar brianteeman brianteeman - close - 27 Mar 2016
avatar wilsonge
wilsonge - comment - 27 Mar 2016

Ah

avatar wilsonge wilsonge - change - 27 Mar 2016
Milestone Added:

Add a Comment

Login with GitHub to post a comment