thoughtbot / ember-formulaic

Simplify form filling in acceptance tests
MIT License
2 stars 1 forks source link

Fallback to missing I18n key as label #9

Closed seanpdoyle closed 9 years ago

seanpdoyle commented 9 years ago

In the case of interpolated labels:

// app/locales/en/translations.js

export default {
  interpolated: 'Remember me ({{name}})',
}
<label for="remember-me">{{t "interpolated" name="Ralph"}}</label>
<input id="remember-me" type="checkbox">

Support falling back to the key value as the label to lookup:

clickOn(t("interpolated", { name: "Ralph" }));

Unfortunately, I18n.t() always returns a string.

We could override this behavior, but it would override it GLOBALLY, which could be confusing for consumers.

calebhearth commented 9 years ago

@seanpdoyle per our conversation around the unexpected case where the label is <label>translated</label>, I'd at least leave a comment in the commit message about it and why we think it's worth the potential hassel.

calebhearth commented 9 years ago

Should the PR message be fillForm and not clickOn?

seanpdoyle commented 9 years ago

Would it be worth adding coverage for fillForm?

It would end up using dynamic properties

fillForm('form', {
  [ (() => t('form.interpolated', { name: 'nested' })) ]: true,
});

// alternatively, we could declare the form fields separate

const fields = {
  'Another Field Label': 'another value',
};
fields[t('form.interpolated', { name: 'nested' })] = true;

fillForm('form', fields);

I thought we decided this was an edge case that would be worth avoiding.

calebhearth commented 9 years ago

LGTM from an API point of view. I'd get someone with opinions about JS to review as well.

calebhearth commented 9 years ago

Would it be worth adding coverage for fillForm?

I didn't realize you were providing clickOn here, so I lean toward no but it's up to you.

calebhearth commented 9 years ago

I thought we decided this was an edge case that would be worth avoiding.

We did, I just misunderstood the commit message and scope of this library.

drapergeek commented 9 years ago

LGTM

I worry about the "Missing Translation" only because you can change that default. This would cause weird behaviors for anyone who overrides that default.

seanpdoyle commented 9 years ago

@drapergeek I agree. I wish the library behaved differently. I don't know of a better, more stable approach

drapergeek commented 9 years ago

@drapergeek I agree. I wish the library behaved differently. I don't know of a better, more stable approach

If we think it's a big enough problem, we could run a test up front to figure out what their fallback is and store that:

const noTranslationText = t('there-is-no-way-anyone-would-have-a-key-named-like-this-because-it-is-super-weird-1234-tttt');
calebhearth commented 9 years ago

because you can change that default.

Can the missing value be retrieved?

No.

Even by intentionally getting an invalid translation, like a uuid?

seanpdoyle commented 9 years ago

I like the idea of stashing the value, unfortunately the default behavior is that that missing key is included in the response, making it unpredictable:

Missing translation: there-is-no-way-anyone-would-have-a-key-named-like-this-because-it-is-super-weird-1234-tttt

!== (and isn't matchable)

Missing translation: the-actual-key
calebhearth commented 9 years ago

I don't care that much, but if you wanted to do it I think you probably could by replacing [known bad translation] with [the key we want] with regexp / whatever JS has for String#sub. That should catch cases where it's not included as well.