User tests: Successful: Unsuccessful:
This just reformats the code on the JLayouts and properly using the API for inline JS and CSS
Make sure that codemirror is still working as expected
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
@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?
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?
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.
@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
I'll do it. But I can't promise the solution won't be slightly controversial.
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
Where are we with this? Ideally I'd like to have this merged in asap
Labels |
Added:
?
|
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.
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
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.
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....
Yeah I wasted a lot of time looking
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?
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!
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-03-02 10:10:17 |
Closed_By | ⇒ | wilsonge |
Milestone |
Added: |
Labels |
Removed:
?
|
Category | ⇒ | Layout |
Status | New | ⇒ | Pending |
Milestone |
Removed: |
@brianteeman Any reason why you reopened this?
It wasnt closed on the issue tracker ;)
Status | Pending | ⇒ | Closed |
Closed_Date | 2016-03-02 10:10:17 | ⇒ | 2016-03-27 11:25:41 |
Closed_By | wilsonge | ⇒ | brianteeman |
Ah
Milestone |
Added: |
Travis please