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

Update Cutting-the-mustard test #4137

Merged
merged 1 commit into from
Feb 18, 2016
Merged

Update Cutting-the-mustard test #4137

merged 1 commit into from
Feb 18, 2016

Conversation

crazytonyi
Copy link

Updates the test conditions so that all of them are checking for Object property using "'propertyName' in Object" pattern. This makes it easier to interpret the individual conditions and what they are checking for, and removes the need for creating an element on the document object simply for testing purposes. Also makes it very clear what the current "cutting-the-mustard" requirements are, eg which properties on which objects need to be present, thus making it easier to unit test and more clear to clients that fail test what is needed to meet minimum requirements.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@crazytonyi
Copy link
Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

'querySelector' in document &&
'addEventListener' in window && Array.prototype.forEach) {
'addEventListener' in window &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Trailing whitespace here that needs to be removed.

@Garbee
Copy link
Collaborator

Garbee commented Feb 17, 2016

LGTM. Just needs the whitespace fixed and the commit ammended.

FYI easiest way to handle that is do the edit. Then git add src/mdlComponentHandler.js && git commit --amend. Walk through saving the commit back. Then git push --force the branch. That way the history is still the single commit as it should be.

Thanks for the PR. It is great to simplify things.

@crazytonyi
Copy link
Author

@Garbee - I totally get what you are saying, except for the part where I actually edit/amend/force-push. Do I need to pull down the repo to my local dev environment, or is this possible via the github browser interface? If I need to put my big boy pants on and use git in local environment, do I need to amend the commit as it exists on the "patch-1" branch and force push it back to the "google" repo as the master? Or am I just hopelessly confused?

@crazytonyi
Copy link
Author

@Garbee - it might be easier than I thought (or even you thought!), as editing the file on the commit gives the option of updating directly on the patch-1 commit or creating new commit/PR. Fingers crossed.

@crazytonyi
Copy link
Author

Or maybe not. Finally got it working with a rebase (to combine the two commits) and a force push. not really sure what all I really did. But it's all one commit again.

@Garbee
Copy link
Collaborator

Garbee commented Feb 17, 2016

I was assuming you were doing things locally. GH (nor any hoster I have seen) provides a good interface to amending existing commits in-browser.

However, running locally will also let you run the unit tests which are failing in the continuous integration check. Until these pass we can't merge any PR. I'll investigate why this is failing when I have time. Nothing in the commit screams failure to me, so there is something subtle going on it seems. My gut says it is related not creating a div to check for classList against.

Updates the test conditions so that all of them are checking for Object property using "'propertyName' in Object" pattern. This makes it easier to interpret the individual conditions and what they are checking for, and removes the need for creating an element on the document object simply for testing purposes. Also makes it very clear what the current "cutting-the-mustard" requirements are, eg which properties on which objects need to be present, thus making it easier to unit test and more clear to clients that fail test what is needed to meet minimum requirements.
@crazytonyi
Copy link
Author

@Garbee - So I finally got it to pass the unit tests by replacing 'classList' in HTMLElement.prototype with 'classList' in document.documentElement. What's odd is that the problem doesn't seem to be the lack of document.createElement('div') , but with the inclusion of HTMLElement.prototype. I also tried Element.prototype and got the same results. If I tried having both, like:

'classList' in document.documentElement && 'classList' in Element.prototype

this would fail, but as soon as the reference to Element.prototype was removed, the mocha test would pass.

Not sure if this reflects a deeper issue where the mocha test (or the general test environment) can't deal with references to the Element prototype, or if this is expected behavior (maybe referencing Element.prototype re-initiates something in the DOM (??)). But I amended this commit to use the document.documentElement as the object to check for classList, since it achieves mostly the same goal.

@Garbee
Copy link
Collaborator

Garbee commented Feb 18, 2016

Yea, anything that supports classList should have it on the document.

cc @jasonmayes for review before merge in case there were any known technical reasons for the original implementation.

@sgomes
Copy link
Contributor

sgomes commented Feb 18, 2016

FYI, I think we should be getting rid of the cutting-the-mustard test in v2. This was originally here as a way of doing graceful degradation for IE9, which didn't support some commonly used methods in our JS. With v2 moving the baseline to IE11, we no longer need to run these checks.

@Garbee
Copy link
Collaborator

Garbee commented Feb 18, 2016

I think we should merge this if it checks out by Jason. We can discuss later whether it should actually be used. I think we may still want to do the check to let developers do progressive enhancement. But that discussion is probably going to derail this PR too much.

Doesn't hurt to have the change in in case we decide to keep it, and if we don't keep the mustard test then the same area is getting deleted. No major harm being done.

@sgomes
Copy link
Contributor

sgomes commented Feb 18, 2016

Sounds good, let's merge for now pending @jasonmayes's OK. LGTM otherwise.

@jasonmayes
Copy link
Member

Taking a look now.

@jasonmayes
Copy link
Member

LGTM.

@Garbee
Copy link
Collaborator

Garbee commented Feb 18, 2016

Thanks Jason!

@crazytonyi Thanks for improving the check.

Garbee added a commit that referenced this pull request Feb 18, 2016
Update Cutting-the-mustard test
@Garbee Garbee merged commit 72c4733 into google:master Feb 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants