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

[Glimmer2] port {{input}} helper tests #13194

Merged
merged 1 commit into from Jun 6, 2016
Merged

[Glimmer2] port {{input}} helper tests #13194

merged 1 commit into from Jun 6, 2016

Conversation

GavinJoyce
Copy link
Member

@GavinJoyce GavinJoyce commented Mar 27, 2016

Part of #13127

UPDATE: This is ready for review

TODO:

  • port main tests
  • port remaining two tests
  • refactor to remove boilerplate
  • 🍏 build (there are some tests failing in saucelabs possible due to a bug with {{input}} on firefox?)
  • annotate/deleted moved tests
  • comment on commented out null tests (comment here)

@GavinJoyce
Copy link
Member Author

I've come across a couple of possible bugs with the {{input}} helper when working on this

[1] https://ember-twiddle.com/07b6f55e92bd89082171afa06d05a2aa?fileTreeShown=false&openFiles=templates.application.hbs%2C

[2] https://ember-twiddle.com/d149c504b7fb9ed30b65e4cfcca10ace?fileTreeShown=false&openFiles=templates.application.hbs%2C

both are related to having initial values of undefined, so they are edge cases

@GavinJoyce
Copy link
Member Author

screen shot 2016-04-04 at 22 43 31

equal(view.$('input').length, 1, 'A single text field was inserted');
});

QUnit.test('should become disabled if the disabled attribute is true', function() {
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to @htmlbars Helpers test: {{input}} : @test dynamic attributes

// this.assertSelectionRange(8, 8); //NOTE: this fails in IE, the range is 0 -> 0 (TEST_SUITE=sauce)
}

['@test specifying `on="someevent" action="foo"` results in a deprecation warning']() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might need to make sure the event is still fired like the old test.

@chadhietala
Copy link
Contributor

Thanks @GavinJoyce! Just one minor nit but it might not be necessary, will defer to @chancancode, @rwjblue or @krisselden. Also for the things you had to comment out can you potentially create a quest issue that summarizes the issues? 🎆 ✨ 🎉

@homu
Copy link
Contributor

homu commented May 22, 2016

☔ The latest upstream changes (presumably e5e6d25) made this pull request unmergeable. Please resolve the merge conflicts.

@homu
Copy link
Contributor

homu commented May 23, 2016

☔ The latest upstream changes (presumably 2a2b59a) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -0,0 +1,444 @@
import { set } from 'ember-metal/property_set';
import TextField from 'ember-templates/components/text_field';
Copy link
Contributor

Choose a reason for hiding this comment

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

please use the helpers, these become const with the flag and we run both @Glimmer and @htmlbars without the flag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify this should be

import { TextField } from '../../utils/helpers';

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, updated

@asakusuma
Copy link
Contributor

What needs to happen to get this merged? @chancancode @rwjblue @krisselden

@rwjblue rwjblue merged commit 1a61ed0 into emberjs:master Jun 6, 2016
@rwjblue
Copy link
Member

rwjblue commented Jun 6, 2016

Looks good to me. We need to cleanup those comments in the first few tests, but that doesn't need to block the rest.

Can someone make an issue and assign to me to clean up these?

@btecu
Copy link
Contributor

btecu commented Jun 7, 2016

@rwjblue is it just removing them? I could submit a pr.

@rwjblue
Copy link
Member

rwjblue commented Jun 7, 2016

@btecu - I'd like to better understand why they don't work in IE, and potentially fix that or just disable those assertions in IE.

@GavinJoyce GavinJoyce deleted the gj/glimmer2-port-input-helper-tests branch June 7, 2016 07:38
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

7 participants