Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

refactor(ngSanitize) Remove workarounds for IE8 #10758

Closed
wants to merge 2 commits into from
Closed

refactor(ngSanitize) Remove workarounds for IE8 #10758

wants to merge 2 commits into from

Conversation

@realityking
Copy link
Contributor

@realityking realityking commented Jan 15, 2015

Also provides better tests.

@realityking realityking changed the title refactor(ngSanatize) Remove workarounds for IE8 refactor(ngSanitize) Remove workarounds for IE8 Jan 15, 2015
@Narretz
Copy link
Contributor

Narretz commented Jan 20, 2015

What do you mean by "better tests"? I assume you removed those tests that were testing the IE8 specific functions?

@Narretz Narretz self-assigned this Jan 20, 2015
@realityking
Copy link
Contributor Author

realityking commented Jan 21, 2015

@Narretz Yes, I removed those testing IE8 specific workarounds and added some tests to tests the general functionality - otherwise there would have been no tests left.

@Narretz
Copy link
Contributor

Narretz commented Jan 21, 2015

Okay, then you should split this into commits - the first includes the new tests, the second removes all IE8 stuff.

@Narretz Narretz added this to the Backlog milestone Jan 21, 2015
@realityking
Copy link
Contributor Author

realityking commented Jan 22, 2015

@Narretz done

@realityking realityking force-pushed the ngSanatize branch 2 times, most recently from ed6b9a6 to 12af2b2 Compare January 25, 2015 14:26
@realityking
Copy link
Contributor Author

realityking commented Jan 30, 2015

@Narretz The only failed job seems to be related to saucelabs. Can this be merged?

@Narretz
Copy link
Contributor

Narretz commented Feb 8, 2015

Hi, sorry for the late response. I have another very small nitpick: our commit messages have a : after the refactor(ngSanitize) part, can you fix that up?

@petebacondarwin are you okay with removing this IE8 workaround?

@realityking
Copy link
Contributor Author

realityking commented Feb 12, 2015

@Narretz Heh, never noticed that. Fixed now.

@petebacondarwin
Copy link
Contributor

petebacondarwin commented Feb 13, 2015

@realityking - more commit message nitpicking :-) We don't capitalize the first letter of the description.

That being said, I am happy to help strip out unnecessary code from the codebase and get this in.
Thanks!

petebacondarwin
petebacondarwin reviewed Feb 13, 2015
View reviewed changes
test/ngSanitize/sanitizeSpec.js
Copy link
Contributor

@petebacondarwin petebacondarwin Feb 13, 2015

Choose a reason for hiding this comment

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

missing semi-colon

@petebacondarwin
Copy link
Contributor

petebacondarwin commented Feb 13, 2015

It is always good to run grunt test on our PRs before submitting. Apart from building, minifying and running all the unit and e2e tests locally, it also runs grunt ci-checks which checks the jshint and jscs rules.

petebacondarwin pushed a commit that referenced this pull request Feb 13, 2015
@realityking @petebacondarwin
refactor(ngSanitize): remove workarounds for IE8
ab7e0cd 
Closes #10758
@realityking
Copy link
Contributor Author

realityking commented Feb 13, 2015

@petebacondarwin I always run grunt test but I hadn't yet discovered grunt ci-checks, thanks for the pointer.

@realityking realityking deleted the ngSanatize branch February 13, 2015 14:22
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
@realityking @netman92
refactor(ngSanitize): remove workarounds for IE8
3ecffe3 
Closes angular#10758
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@Narretz Narretz

Labels

cla: yes component: ngSanitize type: refactor

Projects

None yet

Milestone

Backlog - won't fix

Development

Successfully merging this pull request may close these issues.

4 participants

@realityking @Narretz @petebacondarwin @googlebot