This repository has been archived by the owner. It is now read-only.

replaceClass? #30

Closed
dylans opened this Issue Dec 28, 2015 · 12 comments

Comments

Projects
None yet
5 participants
@dylans
Member

dylans commented Dec 28, 2015

Should we add a replaceClass method? We currently have toggleClass, addClass, and removeClass, so presumably replaceClass would just be a shortcut for removeClass + addClass?

@kfranqueiro

This comment has been minimized.

Show comment
Hide comment
@kfranqueiro

kfranqueiro Dec 28, 2015

Member

IIRC the main reason this existed in Dojo 1 was to improve performance in old IE, which is now a moot point.

Each of the methods currently in the dom module have direct analogues within DOMTokenList. So the question then becomes whether we want to make an exception and go beyond that for this one.

FWIW, I always found domClass.replace in Dojo 1 to be utterly confusing because of its argument order: replace(node, add, remove). If we did add this I would reverse the last two arguments to replaceClass(node, remove, add) (i.e. "replace X with Y").

I'm generally -1 on adding it to begin with, though. It's just a terser and arguably less-clear (due to needing to remember argument order) form of calling removeClass and addClass and doesn't really provide value.

Member

kfranqueiro commented Dec 28, 2015

IIRC the main reason this existed in Dojo 1 was to improve performance in old IE, which is now a moot point.

Each of the methods currently in the dom module have direct analogues within DOMTokenList. So the question then becomes whether we want to make an exception and go beyond that for this one.

FWIW, I always found domClass.replace in Dojo 1 to be utterly confusing because of its argument order: replace(node, add, remove). If we did add this I would reverse the last two arguments to replaceClass(node, remove, add) (i.e. "replace X with Y").

I'm generally -1 on adding it to begin with, though. It's just a terser and arguably less-clear (due to needing to remember argument order) form of calling removeClass and addClass and doesn't really provide value.

@dylans

This comment has been minimized.

Show comment
Hide comment
@dylans

dylans Dec 28, 2015

Member

lol, the argument order bug is what led me to open this one. The only purpose of it is basically less to type, if there's a situation where switching UI state is done by swapping out class names.

Member

dylans commented Dec 28, 2015

lol, the argument order bug is what led me to open this one. The only purpose of it is basically less to type, if there's a situation where switching UI state is done by swapping out class names.

@vansimke

This comment has been minimized.

Show comment
Hide comment
@vansimke

vansimke Mar 9, 2016

Contributor

I'm thinking that there might be a middle of the road approach here. I've started working on an small change to addClass and removeClass functions that will allow them to be chained together. This will eliminate the need for a replaceClass function and prevent the argument confusion that was mentioned. My plan is to return something that looks like this from both of those functions:

    return {
        addClass: (...classes:string[]) => {
            return addClass.apply(null, [element, ...classes]);
        },
        removeClass: (...classes: string[]) => {
            return removeClass.apply(null, [element, ...classes]);
        }
    }

I'm hoping to extend the unit tests to confirm that this works and then I'll submit a PR for a more formal review.

Contributor

vansimke commented Mar 9, 2016

I'm thinking that there might be a middle of the road approach here. I've started working on an small change to addClass and removeClass functions that will allow them to be chained together. This will eliminate the need for a replaceClass function and prevent the argument confusion that was mentioned. My plan is to return something that looks like this from both of those functions:

    return {
        addClass: (...classes:string[]) => {
            return addClass.apply(null, [element, ...classes]);
        },
        removeClass: (...classes: string[]) => {
            return removeClass.apply(null, [element, ...classes]);
        }
    }

I'm hoping to extend the unit tests to confirm that this works and then I'll submit a PR for a more formal review.

@tomdye

This comment has been minimized.

Show comment
Hide comment
@tomdye

tomdye Mar 9, 2016

Member

Wasn't the idea of replaceClass to only touch the Dom classlist once though? chaining add and remove together would perform two operations

Member

tomdye commented Mar 9, 2016

Wasn't the idea of replaceClass to only touch the Dom classlist once though? chaining add and remove together would perform two operations

@tomdye

This comment has been minimized.

Show comment
Hide comment
@tomdye

tomdye Mar 9, 2016

Member

+1 for replaceClass(node, remove, add)

Member

tomdye commented Mar 9, 2016

+1 for replaceClass(node, remove, add)

@vansimke

This comment has been minimized.

Show comment
Hide comment
@vansimke

vansimke Mar 9, 2016

Contributor

@tomdye Its true that this would hit the classlist twice, but I'm not sure how much we're giving up by doing that. What advantage do you see to having a single op?

From my perspective, there is a downside to replaceClass in that it couldn't be variadic. In order to make the API consistent with add/removeClass I think we'd have to accept single values as well as arrays. That isn't a huge deal to implement, but would expose a different interface type to the consumers.

Contributor

vansimke commented Mar 9, 2016

@tomdye Its true that this would hit the classlist twice, but I'm not sure how much we're giving up by doing that. What advantage do you see to having a single op?

From my perspective, there is a downside to replaceClass in that it couldn't be variadic. In order to make the API consistent with add/removeClass I think we'd have to accept single values as well as arrays. That isn't a huge deal to implement, but would expose a different interface type to the consumers.

@tomdye

This comment has been minimized.

Show comment
Hide comment
@tomdye

tomdye Mar 9, 2016

Member

For operations where styling is done correctly and class changes cause cascading changes to child nodes it could cause large re-draws in some cases. I don't personally see the problem with adding the function, it was there for dojo1.x so why not for 2?

replaceClass: (node: HTMLElement, remove: string[] | string, add: string[] | string) : void ?

Apologies, my TypeScript is shady

Member

tomdye commented Mar 9, 2016

For operations where styling is done correctly and class changes cause cascading changes to child nodes it could cause large re-draws in some cases. I don't personally see the problem with adding the function, it was there for dojo1.x so why not for 2?

replaceClass: (node: HTMLElement, remove: string[] | string, add: string[] | string) : void ?

Apologies, my TypeScript is shady

@kfranqueiro

This comment has been minimized.

Show comment
Hide comment
@kfranqueiro

kfranqueiro Mar 9, 2016

Member

it was there for dojo1.x so why not for 2?

I already explained this in my first comment: it was mainly added to Dojo 1 to enable performance improvements in old IE. We no longer care about old IE.

I'm currently still tentatively -1 on this based on my points above (its original purpose is obsolete, it goes beyond the original intended scope of providing analogues to DOMTokenList) and the fact that it's way easier to add APIs later if we find actual demand for them than to remove or change them.

Member

kfranqueiro commented Mar 9, 2016

it was there for dojo1.x so why not for 2?

I already explained this in my first comment: it was mainly added to Dojo 1 to enable performance improvements in old IE. We no longer care about old IE.

I'm currently still tentatively -1 on this based on my points above (its original purpose is obsolete, it goes beyond the original intended scope of providing analogues to DOMTokenList) and the fact that it's way easier to add APIs later if we find actual demand for them than to remove or change them.

@tomdye

This comment has been minimized.

Show comment
Hide comment
@tomdye

tomdye Mar 9, 2016

Member

Fair

Member

tomdye commented Mar 9, 2016

Fair

@vansimke

This comment has been minimized.

Show comment
Hide comment
@vansimke

vansimke Mar 9, 2016

Contributor

Well, at this point, I see three ways to move forward:

  • kill it
  • keep it with replaceClass
  • chain it

Personally, I don't think I've ever used replaceClass and @kfranqueiro makes a good point that we can always add it later. It's really pretty trivial to put this in, but once we add it, it can't be easily removed.

I propose that we close this issue without implementing replaceClass and revisit it if we start to see use cases where it would provide a significant benefit.

@kitsonk do you have an opinion?

Contributor

vansimke commented Mar 9, 2016

Well, at this point, I see three ways to move forward:

  • kill it
  • keep it with replaceClass
  • chain it

Personally, I don't think I've ever used replaceClass and @kfranqueiro makes a good point that we can always add it later. It's really pretty trivial to put this in, but once we add it, it can't be easily removed.

I propose that we close this issue without implementing replaceClass and revisit it if we start to see use cases where it would provide a significant benefit.

@kitsonk do you have an opinion?

@kitsonk

This comment has been minimized.

Show comment
Hide comment
@kitsonk

kitsonk Mar 9, 2016

Member

I agree with @kfranqueiro the only reason far it was to not clobber old IE. The utility of it was always limiting, as it required you think about how you could optimize your operations, not because it was the natural way you dealt with classes. We should always err on the side of keeping the API surface as small as possible.

Even if we wanted something like that, I would say we would be better suited trying to adopt other common paradigms in JavaScript, like splice instead of replace.

Member

kitsonk commented Mar 9, 2016

I agree with @kfranqueiro the only reason far it was to not clobber old IE. The utility of it was always limiting, as it required you think about how you could optimize your operations, not because it was the natural way you dealt with classes. We should always err on the side of keeping the API surface as small as possible.

Even if we wanted something like that, I would say we would be better suited trying to adopt other common paradigms in JavaScript, like splice instead of replace.

@vansimke

This comment has been minimized.

Show comment
Hide comment
@vansimke

vansimke Mar 9, 2016

Contributor

Okay, then I'll shelve this issue for now. I'd close it, but I think something is wrong with my permissions; I can't assign issues or PRs and I also can't change statuses.

Contributor

vansimke commented Mar 9, 2016

Okay, then I'll shelve this issue for now. I'd close it, but I think something is wrong with my permissions; I can't assign issues or PRs and I also can't change statuses.

@kitsonk kitsonk closed this Mar 9, 2016

@dylans dylans added this to the long-grass milestone Oct 27, 2016

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