Remove UserAgent from DomHelper, replace with feature detection #50

Merged
merged 5 commits into from Apr 20, 2012

Conversation

Projects
None yet
2 participants
@seanami
Contributor

seanami commented Apr 19, 2012

UserAgent was required to construct a DomHelper, but the only thing it was used for is in setStyle to determine if we're in an old IE and need to use a different approach. I was using DomHelper in some code and finding it cumbersome to have a UserAgent instance around as well. This goal is better accomplished with a feature-detection approach anyway.

I've replaced the UserAgent arg with feature detection for getting/setting the style attribute. This is a very similar approach to the one that jQuery uses to detect the same feature.

This has the added side-effect of allowing many of the modules to no longer construct a UserAgent instance using the parser, since they only used it for the DomHelper, which simplifies things.

@jeremiele and @rcarver, I'd especially love your feedback on this change and the code.

Sean McBride
Remove UserAgent from DomHelper, replace with feature detection
UserAgent was required to construct a DomHelper, but the only thing it was
used for is in setStyle to determine if we're in an old IE and need to use a
different approach. I was using DomHelper in some code and finding it cumbersome
to have a UserAgent instance around as well. This goal is better accomplished
with a feature-detection approach anyway.

I've replaced the UserAgent arg with feature detection for getting/setting the
style attribute. This is a very similar approach to the one that jQuery uses to
detect the same feature.

This has the added side-effect of allowing many of the modules to no longer
construct a UserAgent instance using the parser, since they only used it for the
DomHelper, which simplifies things.
@rcarver

This comment has been minimized.

Show comment Hide comment
@rcarver

rcarver Apr 19, 2012

Contributor

👍

Contributor

rcarver commented Apr 19, 2012

👍

Sean McBride
Move feature detection out of the constructor
Otherwise, the JS can't run in environments that don't have a DOM
implementation, which makes things difficult.
@seanami

This comment has been minimized.

Show comment Hide comment
@seanami

seanami Apr 19, 2012

Contributor

I had to make one additional change after using this in practice. Having the feature detection triggered in the constructor means that the JS environment has to have a DOM implementation just to construct a DomHelper. This makes it difficult to test in JS environements wrapped within other languages, like Johnson in Ruby.

Unfortunately, it means you have to remember to call a method on your DomHelper instance after instantiating it for normal use. I'm open to other suggestions on how to improve this.

Contributor

seanami commented Apr 19, 2012

I had to make one additional change after using this in practice. Having the feature detection triggered in the constructor means that the JS environment has to have a DOM implementation just to construct a DomHelper. This makes it difficult to test in JS environements wrapped within other languages, like Johnson in Ruby.

Unfortunately, it means you have to remember to call a method on your DomHelper instance after instantiating it for normal use. I'm open to other suggestions on how to improve this.

@seanami

This comment has been minimized.

Show comment Hide comment
@seanami

seanami Apr 19, 2012

Contributor

Actually, scratch that. This doesn't really help my case after all, and makes things to complicated in the normal case. I'll think of something better.

Contributor

seanami commented Apr 19, 2012

Actually, scratch that. This doesn't really help my case after all, and makes things to complicated in the normal case. I'll think of something better.

Sean McBride added some commits Apr 19, 2012

Sean McBride
Use a "feature detect on first call" style to avoid constructor DOM
This is a less onerous way to avoid DOM construction caused by the DomHelper
constructor. We make a method to check if a feature is supported and memoize
the value the first time it's called.
@seanami

This comment has been minimized.

Show comment Hide comment
@seanami

seanami Apr 19, 2012

Contributor

Alright, I think I now have this to a point where I'm happy with the implementation and it works well in the target situation. Ready for more review.

Contributor

seanami commented Apr 19, 2012

Alright, I think I now have this to a point where I'm happy with the implementation and it works well in the target situation. Ready for more review.

seanami pushed a commit that referenced this pull request Apr 20, 2012

Sean McBride
Merge pull request #50 from typekit/sm-domhelper-update
Remove UserAgent from DomHelper, replace with feature detection

@seanami seanami merged commit 552ad81 into master Apr 20, 2012

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