Skip to content
This repository has been archived by the owner on Feb 11, 2021. It is now read-only.

Add instead of override? #18

Closed
slavafomin opened this issue Feb 11, 2016 · 17 comments
Closed

Add instead of override? #18

slavafomin opened this issue Feb 11, 2016 · 17 comments

Comments

@slavafomin
Copy link

Hello!

Thank you for this module!

Is it possible to have hierarchical CSS files applied to the final state or the last state will re-define the entire list? I have application with multiple hierarchical layouts and I want to have global CSS files for entire layouts as well as custom CSS files for some states. I want to keep things DRY of course, i.e. not to re-define the entire list for each "leaf" state.

Thank you!

@slavafomin slavafomin changed the title Add instead override? Add instead of override? Feb 11, 2016
@slavafomin
Copy link
Author

Hmm, I've experimented with it and now experiencing an opposite problem.

I have the topmost state:

.state({
  name: 'root',
  abstract: true,
  data: {
    css: ['/css/application.css']
  }
}

And the leaf state:

.state({
  name: 'foo',
  parent: 'root',
  data: {
    css: ['/css/foo.css']
  }
})

And for some reason when I load the foo state both application.css and foo.css are loaded. How do I override the application.css with foo.css?

Thanks!

@manuelmazzuola
Copy link
Owner

And for some reason when I load the foo state both application.css and foo.css are loaded. How do I override the application.css with foo.css?

Yes because the plugin works like you've described in the issue.
If the ancestors of the current state have some css defined they are added to the current state's css.

In this example the parent-css.css is loaded by the all states
http://plnkr.co/edit/Vb1fvndk4mFLfC4hyMGv?p=preview

@manuelmazzuola
Copy link
Owner

I can add an option to change the plugin behaviour, can be a solution?

@slavafomin
Copy link
Author

Yes, this could be a solution for my concrete example, but then we will have another issue where we will want to add instead of override.

I've got an idea of named styles, I've described it here: castillo-io/angular-css#55 (comment)

This would be the best possible solution in my opinion.

@slavafomin
Copy link
Author

However, I will appreciate the quick fix if it's possible = )

@manuelmazzuola
Copy link
Owner

Here a working version that implements your named styles idea.
http://plnkr.co/edit/HIcYEj2QRqBCwbZCU0Il?p=preview

I'll write tests and release the new version until tonight (here GTM+1).

@slavafomin
Copy link
Author

Wow, that was fast = )
Thank you, I will give it a try!

@manuelmazzuola
Copy link
Owner

👍

@slavafomin
Copy link
Author

I've tested the latest version of the module in our project and it works amazing! Thanks again @manuelmazzuola, great work! Right now, it's the best conditional stylesheet module for Angular.js for sure. We're going to use it in all of our projects.

However, I've stumbled upon some other good ideas of how it could be improved. I'm going to share them with you:

1). There is no direct way to disable some named style in the state down the chain. Consider this example:

// The root state:
.state({
  name: 'root',
  abstract: true,
  data: {
    css: [
      {
        name: 'layout',
        href: 'primary-layout.css'
      },
      {
        name: 'fancybox',
        href: '/vendor/fancybox/jquery.fancybox.css'
      },
      {
        name: 'intl-tel-input',
        href: '/vendor/intl-tel-input/css/intlTelInput.css'
      }
    ]
  }
})

// Some child state
.state({
  name: 'bar',
  parent: 'root.foo'
  data: {
    css: [
      {
        name: 'layout',
        href: 'another-layout.css'
      },
      {
        name: 'fancybox',
        href: 'empty.css'
      },
      {
        name: 'intl-tel-input',
        href: 'empty.css'
      }
    ]
  }
})

Right now I have to use empty.css empty file to disable some named styles. It would be better to just pass null to the href attribute in order to disable it.

2). Named styles construct is too bulky, what if we could support a minimalistic hash notation?

.state({
  name: 'root',
  abstract: true,
  data: {
    css: {
      layout: 'primary-layout.css',
      fancybox: '/vendor/fancybox/jquery.fancybox.css',
      extended: {
        // name: 'extended' implied
        href: 'extended.css',
        'some-other-property': 'value',
        'some-more-property': 100500
      },
      $unnamed: [
        // Some other unnamed styles:
        'foo.css',
        'bar.css',
        'baz.css'
      ]
    }
  }
})

3). What if we could advance the idea of named styles further and create a named groups of styles? Often, the layout consists not only of the single css file, but also requires some other vendor files. Please consider this example:

.state({
  name: 'root',
  abstract: true,
  data: {
    css: {
      layout: [
        // All CSS files of the primary layout are grouped together under the "layout" named group
        'bootstrap.css',
        'primary-layout.css',
        '/vendor/fancybox/jquery.fancybox.css'
      ],
      overall: [
        // The entry of the named group list could be either a string as above
        // or a hash-object with additional options
        {
          href: 'animate.css',
          'some-other-property': 'value',
          'and-another-property': 100500
        }
      ]
    }
  }
})
.state({
  name: 'bar',
  parent: 'root.foo',
  data: {
    css: {
      // Overloading the entire layout by a single CSS file
      layout: 'alternative-layout.css'
    }
  }
})

Actually, I think that named groups approach will be able to replace named styles and unnamed styles entirely. In my opinion, it's very flexible approach that will allow developers to build a very robust layout schemes.

I'm sorry I couldn't help with the implementation of this ideas right now, however I will be very glad to rewrite the README and to write some beautiful and comprehensive documentation for this module if you choose to implement those ideas. Probably I could ask our designer to even draw some logo for this module and to craft a small website if required.

What do you think? Cheers!

@manuelmazzuola
Copy link
Owner

I'm glad to see that this module is useful to you and to other people.
I think that your latest example is the right proposal to improve this module.
The array of strings is enough for a basic usage, and the object approach is the best one for an advanced usage: the user will be able to build a custom and flexible layouts schemas.

I'll try to implement those enhancements in these days.
Pull requests to modify the readme and the gh-pages branch are very appreciate 👍

Thank you @slavafomin for the ideas 👍

@slavafomin
Copy link
Author

Thank you! I will be looking forward for the new features in order to test and verify them. After that I will be glad to update the docs. And yet after that, you will be able to ship a new major release, 2.0.0 I believe?

By the way, why don't we move css property from data and put it on the same level as data? Or is it against the ui-router's recommendations? I think it will make configuration even more clear and minimalistic.

Also, why do this module require the directive to operate? Is it possible to remove the directive? It will make configuration easier and even more straightforward. Actually, I've forgot to add the directive earlier today and spent almost 20 minutes trying to figure out why the module is not working = )

What is the value of using the directive from the end user perspective?

@manuelmazzuola
Copy link
Owner

Well, angular recommendations say that the DOM should be modified only by a directive, and I think they are right so a directive is necessary, what do you think about it?

The ui-router's doc says to use a data attribute to avoid conflicts with other modules but we can move up one level the CSS property and make it a sibling of data property.

@slavafomin
Copy link
Author

I totally agree with the recommendation regarding the directives, it makes sense from the abstract perspective: if every module will manipulate entire DOM of the document on it's own accord, it will bring chaos to the application. However, in our case, the manipulation is very limited and highly predictable. I didn't think it will hurt someone in practice, on contrary it will make the life easier. In my opinion it's totally safe to get rid of the directive in our concrete case.

However, if you want to play strict and by the rules, then the directive should be placed where you want your link elements to appear, e.g.:

<html>
  <head>
    <title>Hello World!</title>
    <link rel="apple-touch-icon" sizes="152x152" href="/apple-touch-icon-152x152.png">
    <link rel="apple-touch-icon" sizes="180x180" href="/apple-touch-icon-180x180.png">
    <ui-router-styles-css-links></ui-router-styles-css-links>
  </head>
  <!-- ... -->
</html>

That will make more sense than putting it on the body element as an attribute.

However, again, according to the specs: A <link> tag can occur only in the head element; so putting it somewhere else doesn't make sense. In the end, I didn't see any possible value for the end user to use this directive at all. I would suggest to drop it and make the life easier.

@slavafomin
Copy link
Author

Regarding the data attribute, it will not help to prevent conflicts with other modules, cause other modules will probably also use the data attribute. It could prevent the future conflicts with the ui-router itself, however, I think that they will never implement such behavior for CSS in the core module. In other words, I think we can safely add our css property on the same level as data, like other modules did. That way we will leave the data property entirely to the application domain, which is good.

@manuelmazzuola
Copy link
Owner

Yes, I agree with you about the css attribute, I'll make it a sibling of the data attribute.
A directive can be placed everywhere so the user can choose to enable this module for a state but not for another, I'm not completely sure to drop it and replace it with a pure service, but maybe you're right.

@slavafomin
Copy link
Author

I think the module should be entirely configured from the router's configuration or through the service provider. I just can't really see any practical benefits of using the directive = )

@manuelmazzuola
Copy link
Owner

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants