Skip to content

chore(data-table): hide stability warning in jasmine #4840

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

Merged
merged 1 commit into from
Jun 6, 2017

Conversation

devversion
Copy link
Member

@devversion devversion commented May 27, 2017

  • No longer shows the data-table stability warning inside of jasmine tests.

@devversion devversion requested a review from andrewseguin May 27, 2017 08:58
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 27, 2017
@andrewseguin
Copy link
Contributor

Looping in Jeremy, he requested the data table warning to be a bit more intrusive to make it clear that it's not yet ready

@jelbourn
Copy link
Member

This isn't part of the sanity checks

@devversion
Copy link
Member Author

Yeah it isn't really a sanity check but it allows us to disable those warnings in tests.

Having the warning super-intrusive just spams the whole tests and doesn't really help at all.

I initially only wanted to show the warning in dev-mode only but after chatting with @crisbeto I agreed that it would be nice if developers can disable the warning though

@jelbourn
Copy link
Member

I'd rather add a check for window.jasmine to disable it so that we just don't log it during our own tests

@devversion
Copy link
Member Author

That would work too. But the issue doesn't just appear in Material. Every developer that writes tests using Material will get all of those data-table errors.

The sanity checks solution would be kind of temporary until this warning will be removed anyways. But for now it's a super intrusive warning.

@jelbourn
Copy link
Member

@devversion the warning is in the constructor of the table- they'll only get it if they use the table, which is exactly the point

@devversion
Copy link
Member Author

@jelbourn Yeah that's clear. But as I mentioned everybody that tests for example a component that contains a md-data-table will get the error. Similar as we do by explicitly testing the md-data-table.

@devversion
Copy link
Member Author

@jelbourn I'm fine doing the window.jasmine thing since you said that nobody should use the data-table at all yet (and it's exposed for the demo-app). Let me know if I should proceed with that.

@devversion devversion force-pushed the fix/data-table-warning branch 2 times, most recently from 0848a5f to 61031ca Compare June 2, 2017 13:50
@devversion devversion changed the title fix(data-table): show warning as part of sanity check chore(data-table): hide stability warning in jasmine Jun 2, 2017
@devversion
Copy link
Member Author

@andrewseguin @jelbourn Made the requested changes. Please review again.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jun 2, 2017
@andrewseguin
Copy link
Contributor

Can you rebase?

* No longer shows the data-table stability warning inside of jasmine tests.
@devversion devversion force-pushed the fix/data-table-warning branch from 61031ca to b71b121 Compare June 5, 2017 22:08
@devversion
Copy link
Member Author

@andrewseguin Done

@andrewseguin andrewseguin merged commit 1f93cdb into angular:master Jun 6, 2017
@devversion devversion deleted the fix/data-table-warning branch November 11, 2017 10:23
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants