Move assets from off-canvas to the js and images directories to take advantage of URL helpers make them more reusable [#2092869]

Comment	File	Size	Author
#7	omega-moved-off-canvass-assets-2092869-7.patch	0 bytes	Internet
#1	moved-offcanvas-assets-2092869-1.patch	4.1 KB	peteainsworth

Comment #1

peteainsworth CreditAttribution: peteainsworth commented 19 September 2013 at 11:43

Status:

Needs review

» Active

File	Size
moved-offcanvas-assets-2092869-1.patch	4.1 KB

I've moved the assets, creating a new js/layouts/off-canvas directory to match the CSS structure. I've also updated _off-canvas.scss and off-canvas.layout.inc to reflect the new locations. Should off-canvas.js be renamed off-canvas.layout.js? What is the convention there?

Log in or register to post comments

Comment #2

peteainsworth CreditAttribution: peteainsworth commented 19 September 2013 at 11:42

Status:

Active

» Needs review

Changed issue status.

Log in or register to post comments

Comment #3

peteainsworth CreditAttribution: peteainsworth commented 19 September 2013 at 11:46

Status:

Active

» Needs review

Also should I include the regenerated css in the patch?

Log in or register to post comments

Comment #4

msmithcti CreditAttribution: msmithcti commented 19 September 2013 at 20:10

Status:

Needs review

» Needs work

Looking like a worthwhile restructure. Just a minor point:

```
+++ b/omega/layouts/off-canvas/off-canvas.layout.inc
@@ -18,4 +18,4 @@
+scripts[] = js/layouts/off-canvas/off-canvas.js
```
Why are we moving this out of the layout? IMO unlike the images this is so specific to the layout it should stay there.

I think you're right, renaming this file to off-canvas.layout.js would seem to follow convention (or at least set a useful convention to follow).

+++ b/omega/sass/layouts/off-canvas/_off-canvas.scss
@@ -10,11 +10,11 @@
+  background: transparent image-url("close.png") center center no-repeat;
...
+  background: transparent image-url("menu.png") center center no-repeat;
 }

Such a nice clean up!

You are right to leave the CSS out of the patch, makes it easier to review.

Log in or register to post comments

Comment #5

peteainsworth CreditAttribution: peteainsworth commented 25 September 2013 at 11:25

There was no particular reason behind moving the JS it was just based on #2090571-4: Create an off canvas Omega layout

Ideally it would be nice to keep all the layout specific assets within the layout directory to contain them. However, since the SASS already needs to be contained within the configured sass-dir, should the JS follow this convention or should the convention be that assets remain in the layout directory unless they can potentially be reused? In this case, I could see the JS being reused in an alternative off-canvas layout.