Improved ng-class documentation #3084

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@xjamundx
Contributor

Many people are complaining that there isn't an example of how to use the "map" syntax. I've updated ng-class example to use the map approach.

@petebacondarwin
Member

@xjamundx - thanks for this PR. I think that it would be best if we showed all the types of syntax that ng-class can take rather than just replacing the string version with the object version. There is also an array syntax too.
Can you update your PR accordingly?
Thanks

@xjamundx
Contributor

Example updated to show an example as seen here:
http://jsfiddle.net/wMfCB/2/

Now the array example is a bit contrived. Let me know if you have any better idea :)

Oh and the E2E test example will be broken. I'll try to get onto that next....

@xjamundx
Contributor

I've updated the docs, but can't figure out how to run the tests locally, they seem to fail on travis on (seemingly) unrelated things.

@xjamundx
Contributor

I've simplified the example and locally the tests (related to my example) all pass. If its failing it's because of a problem with master. See example here:
http://jsfiddle.net/wMfCB/14/

@jeffbcross
Contributor

@xjamundx some thoughts:

  • Can you make sure all newlines are consistently tabbed two spaces from their parent?
  • Can you think of a way to visualize the array syntax better in the demo, or make it more interactive? Maybe you could add some text beneath the "Using Array Syntax" text to better illustrate what's happening, like: ng-class="['bold', 'strike', 'red']". But if you add this helper once, it should be added for all three examples.
  • I'm not sure any value is added by separating the properties into script.js, since it's one more place I have to look to understand the example. What is the reason?
  • I'm pushing a fix for the failing test shortly.
@xjamundx
Contributor

Thanks for the great feedback, I'll keep messing with it!

@jeffbcross
Contributor

Cool! If you update the PR, can you tag me in a comment so I'll be sure to get a notification?

@xjamundx
Contributor

Okay, I left the demo basically the same, but I believe by combining most of the code into the HTML it becomes much more apparent what's going on and is hopefully a more useful example.

@jeffbcross Adding an interactive array example I think was non-trivial and would complicate the example. I was going to do something with select boxes and letting people remove them, but I couldn't figure out how to do it without adding back a <script> which is way less fun :-p Any other ideas?

@xjamundx
Contributor

I have an idea for a decent example....multiple text boxes. I'll give it a shot.

@xjamundx
Contributor

Let me know what you think about the new version @jeffbcross

@jeffbcross
Contributor

Will do! I'll take a look once I wrap up another issue.

@xjamundx
Contributor

Any feedback yet @jeffbcross. I'd love to get this merged if possible :)

@jeffbcross
Contributor

@xjamundx it looks like you need to rebase against master. There were some upstream changes that are causing conflicts.

@xjamundx
Contributor

Okay so I did rebase and removed the new animation stuff. Obviously it may be a requirement to add it back in, but in my opinion it makes the actual class demo way more confusing and for my demo it becomes 9x more complicated (3x as many classes per class)......so let me know if you want me to re-think my demo to add animations or if this will work. Thanks @jeffbcross

@kumarharsh

Perhaps add an "expression" syntax too?

bold === true ? 'bold' : "italic"
@jeffbcross
Contributor

@xjamundx @ksheedlo and I reviewed the latest, and we like the way this works now, but the animation example needs to be included, not removed. I'm not sure where the complexity lies with including animations AND your new examples, but could you find a simple way to have both?

@IgorMinar IgorMinar closed this in 66007a4 Aug 7, 2013
@xjamundx
Contributor
xjamundx commented Aug 8, 2013

yay thanks for merging this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment