Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider prefixing animating class #194

Closed
kpeatt opened this issue Jul 18, 2014 · 19 comments
Closed

Consider prefixing animating class #194

kpeatt opened this issue Jul 18, 2014 · 19 comments

Comments

@kpeatt
Copy link

kpeatt commented Jul 18, 2014

I tend to prefer to prefix these types of classes whenever possible to avoid conflicts with existing class names. While the chance of running into a conflict with animating is probably low, this could be entirely mitigated by making the class velocity-animating or something simpler. Thoughts?

@julianshapiro
Copy link
Owner

That's what I was originally planning on doing before I realized it was inelegant/verbose, too library-specific (anything could be causing an animation and might want to add the class), and that :animating (with a colon) wasn't usable with querySelectorAll.

I do think chances of conflict with animating are low. It was a risk I was willing to take. But I am open and eager to hear good alternatives.

Would love for more people to chime in on this.

@kpeatt
Copy link
Author

kpeatt commented Jul 18, 2014

Yeah, I think that's fair. I always err on the extreme side of caution with this because we operate with a lot of other people's markup and scripts most of the time so I don't like to leave it to chance. That said, my use case is not common and when more control is present it's verging on being completely unnecessary. What about a data attribute?

@Rycochet
Copy link
Collaborator

Options I can see -

  • Stick with "animating" (no option)
  • Have a prefix "velocity-animating" (no option)
  • Have a default as above, but give an option for your own classname
  • Have no class added, but give an option for your own classname
  • Use a data-animating or data-velocity attribute - or even a custom one via option

Personally I'd prefer having an option in there since it'd make things easier (even a data attribute can be easily checked for)... In fact, I did just have a wonder - is there an ARIA data tag to note when something is animated?

@julianshapiro
Copy link
Owner

Great suggestions, Robin.

Not sure what to do here. Pinging @rosslavery, @Oliboy50, @joshuasmock, @ydaniv, @nikola, @bhongy, @mhawk0, @codedependant, @DevertNet for advice.

Be sure to take a look at #190 before responding.

@nikola
Copy link
Contributor

nikola commented Jul 18, 2014

Personally, I tend to prefer exposing as many internal constants as possible for overloading via a public interface. Therefore I vote for "Have a default [i.e. velocity-animating], but give an option for your own classname".

@rosslavery
Copy link

I am definitely of the belief that the class should be namespaced with something.

Whether it's velocity- v- vl-, whatever. Anything to avoid clashes, and also to make it slightly easier for developers new to a project to track down what is adding classes is a good thing to me.

I wouldn't personally need an option, but based on other's comments it seems like they'd like one so that would be my preference.

velocity-animating with an option to change it.

@julianshapiro
Copy link
Owner

Ok. Thanks, guys. I will switch this to velocity- in the next release.

Sent from my iPhone

On Jul 18, 2014, at 11:01 AM, Ross Lavery [email protected] wrote:

I am definitely of the belief that the class should be namespaced with something.

Whether it's velocity- v- vl-, whatever. Anything to avoid clashes, and also to make it slightly easier for developers new to a project to track down what is adding classes is a good thing to me.

I wouldn't personally need an option, but based on other's comments it seems like they'd like one so that would be my preference.

velocity-animating with an option to change it.


Reply to this email directly or view it on GitHub.

@jo-sm
Copy link

jo-sm commented Jul 18, 2014

I think that the class should be namespaced with velocity- or something analogous. It'll follow the conventions that you've already set for Velocity like not overwriting jQuery's animate function. I don't know if it's necessary to give the option to users to change it though. I would err on the side of consistency and ease of debugging by not giving users an option.

@julianshapiro
Copy link
Owner

Agreed. I don't plan to allow an option.

@nikola
Copy link
Contributor

nikola commented Jul 18, 2014

Ok, but you already have a $.Velocity.defaults map, anyway:

    /*****************
        Constants
    *****************/

    var NAME = "velocity",
        DEFAULT_DURATION = 400,
        DEFAULT_EASING = "swing";

     /* Page-wide option defaults, which can be overriden by the user. */
        defaults: {
            queue: "",
            duration: DEFAULT_DURATION,
            easing: DEFAULT_EASING,
            // etc.
        }

I'd suggest to declare the default class name as a constant (see above), and allow users to override it through $.Velocity.defaults, e.g.

var ANIMATING_CLS = NAME + "-animating";

defaults: {
    animatingClassName: ANIMATING_CLS
    // etc.
}

@Rycochet
Copy link
Collaborator

I like @nikola's suggestion actually - wouldn't be an option per-se, but it would be there if someone really did want to change it (ie, I try to keep everything namespaced the same, so being able to put velocity into the same namespace would be nice, but totally non-essential - and it's also nice to be able to use it as an object name, rather than looking for a static string)

@bhongy
Copy link

bhongy commented Jul 20, 2014

I agree with @Rycochet and @nikola. I cannot see that I would need to change it (but there might be the cases when working with another library, say, scrolling or Physics engine). This adds flexibility and the code is really not getting more complex.

@julianshapiro
Copy link
Owner

It's now velocity-animating. I might add support for user-defined animating classes in the future. I will keep it as it is for now until I get requests. Thanks for your input, everyone. MUCH LOVE! :-D

@kpeatt
Copy link
Author

kpeatt commented Jul 24, 2014

<3 Thanks Julian.

@hugocaillard
Copy link

You could actually use some BEM (block, element, modifier) conventions. Such as:

.main-block (blocks take a dash)
.element
.-modifier (starting with a dash)

Of course you would mainly have modifiers. But the BEM strategy is kind of a growing thing to do maintainable and readable CSS. The thing is that we see different BEM syntaxes but the one above is pretty easy.
So basically, .-animating could be a good alternative. Or the "official" BEM syntax uses two dashes (.modifier--) but it's a bit heavy I think.

Well, I'm kind of late in the conversation but I just wanted to talk a bit about it, because BEM might become a big thing in the future of CSS. :)

@julianshapiro
Copy link
Owner

Hey @hugocaillard! Great point :) I like -animating... surprised I didn't think of that earlier. Will chew on this.

@kpeatt
Copy link
Author

kpeatt commented Jul 24, 2014

I think -animating has almost the exact same issue that just animating does. The point of the prefix is to own this class name and -animating is, with the proliferation of this style of BEM — one that I know is used on our product team at Mobify — only going to become more likely to conflict. From my perspective, the prefixed method is more viable.

@julianshapiro
Copy link
Owner

True that.

@DevertNet
Copy link

I think its better to use velocity-animating with an option to change that class. So you are full flexibility ;)

Rycochet pushed a commit that referenced this issue Aug 3, 2020
Switch “animating” indicator class to “velocity-animating”. Closes #194.

Support for passing hex values into color properties. (Previously, you
could only pass in individual RGBA components.) Closes #179.

Fixes bug where slide functions wouldn’t reset elements’ height to
auto. Closes #183.

Support for getting border-color in Firefox. Closes #196.

Fixes inability to animate `y2` on SVG elements. Closes #199.

Allow passing in explicit `display: none` into an Out UI pack
transition. Closes #201.

Fixes vw/vh unit support for `translateX/Y`. Closes #181.

Fixes bug where `delay` couldn’t be used with `stagger`. Closes #202.
Thank you, @Guanche.

Fixes bug where original call’s easing type wouldn’t be re-used for a
`reverse` command.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants