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

v2 #38

Merged
merged 69 commits into from
Jan 26, 2015
Merged

v2 #38

merged 69 commits into from
Jan 26, 2015

Conversation

shawnbot
Copy link
Owner

v2 replaces several of its old incomplete or non-compliant shims with @WebReflection's excellent ie8 and dom4 libraries (both referenced now as submodules, as is es5-shim), and introduces an actual test suite that runs in both IE8 and modern browsers. Some things to do before merging/releasing:

And, because this all started as a way to get IE8 working with d3, let's also consider doing the following:

ElementPrototype,
{
// bonus
textContent: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Node.prototype.textContent shim I added in #27 was more complete and necessary for reasons outlined in the issue. This one creates a regression with non-Element nodes and re-breaks, for example, certain common uses of jQuery.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to update ie8 with your Node.prototype.textContent proposal, can you point me at the shim you are mentioning? Thank you

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's basically in the pull request I referenced, or you can just look at the element-properties.js file in the "js" directory of the current version. I'm also planning on making a pull request to add it to polyfill, which would also include tests. (Please also note that I haven't tested on IE 6 or 7, as those were not target browsers, and I'm not sure it will apply to them.)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks guys. @WebReflection, let me know when this is done in ie8 and I'll update here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be done in 0.2.1br returns \n and everything else should be patched as it was here (well, more or less, it's optimized/retested code)

Please let me know if there's any problem with such version. Regards

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if standards have those as enumerable I will … setters returning values are misleading for the reason you might return something different and expect that value but the operation you thought was the result of a setter will break such expectations ;-)

var o = {};
Object.defineProperty(o, 'test', {set: function(){return 123}});
var value = (o.test = 456);

// you see what I mean ?
value // 456

If it's style, drop it, 'cause it's a misleading one. Cheers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite right! I meant it's a style I use in certain other contexts where I might actually want to return something useful from a set operation. (Though, really, here I just blindly copied what the original author did.)

On the subject of copying what others do, some quick testing shows that textContent is both enumerable and configurable (or at least the property can be deleted) in modern browsers, though I haven't found anything about it in specs.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made them configurable but if I try to make them enumerable too everything fails. I think configurable only might be OK t we don't have many options here … I also wrote a comment explaining this so enumerable: false it is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out neither configurable nor enumerable are used in IE8 anyway (according to Microsoft--see "Requirements" section near bottom), even though enumerable: false is known to cause an error in IE8 for accessor descriptors such as this one... which you discovered the hard way. :] In any case, I think what you've done is good to go.

(Just out of curiosity, were you testing in standards mode or compatibility mode (or both) when you got the error?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all tests against this page, bear in mind it's interactive (and yes, some browser such safari fails 'cause not fully compliant with W3C specs)

tests are usually under standard mode as quirk one I believe is not a real-world scenario anymore (hopefully)

@shawnbot
Copy link
Owner Author

@WebReflection
Copy link

@shawnbot current ie8 textContent is as standard as it was with previous element-prototype file

@shawnbot
Copy link
Owner Author

Thank you so much, guys! I'll update the submodule as soon as I'm able and check off that item. @usmonster, can you suggest some unit tests for this?

@WebReflection
Copy link

@shawnbot all new entries in the textContent property for all constructors prototypes have been added as tests in ie8, not sure you should re-test in here ;-)

@shawnbot
Copy link
Owner Author

Great, thanks!

@shawnbot shawnbot changed the title V2 v2 Nov 17, 2014
@lusse
Copy link

lusse commented Jan 8, 2015

is this going to get merged into master or not? aight.d3.js isn't working with the latest d3 version. I'm guessing this version does?

shawnbot added a commit that referenced this pull request Jan 26, 2015
@shawnbot shawnbot merged commit f2011af into master Jan 26, 2015
@shawnbot shawnbot deleted the v2 branch January 29, 2015 19:02
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

Successfully merging this pull request may close these issues.

None yet

4 participants