Allow an empty defaultValue option for enhanceSelectElement

[lennym]

The check for a default value option matches false-y values, but that also catches the case where the default value is explicitly set to an empty string.

Add an extra condition for the case where the default value is '' so that it is not overwritten with the content of the first <option>.

[tvararu]

Good find! Are you happy to add a functional test (this one is a good reference) and a CHANGELOG line so I can merge and ship this straight away?

[lennym]

Very happy. Tests were on their way, but hit a slight blocker in that I was struggling to get them to run locally, and I was planning a separate PR to resolve my test issues. I'll get on the case though.

[lennym]

Turns out most of my test blockers were my own out-of-date installs (my local firefox was 23 major versions behind current!).

I've added a test above, but it seems to be failing, and I'm not 100% sure why. I'll go into it a bit further, but I haven't used karma in a long time, or any react testing tools, so it might be a while. Any insight you might be able to provide would be much appreciated.

Got there in the end!

[tvararu]

@lennym well done! CI didn't succeed for one reason or another, I've ordered a rebuild. Will merge after.

[lennym]

I saw. Thanks 💯 for your super fast help on this.

[tvararu]

@lennym sorry about this, CI seems to be having trouble connecting to Sauce Labs for the integration tests. It will take a bit longer to merge this while I investigate.

I suspect it may be related to the fact that this PR is coming from an external fork, in which case I'd really like to fix this issue.

[lennym]

No problem. I know there are issues in travis with secure environment variables not being available for PR builds to avoid them being vulnerable to malicious PRs.

There's no urgency on our side at all. I just took on the spike to implement this as a "Friday afternoon job" - we're not necessarily planning on putting into production any time soon.

[tvararu]

No problem. I know there are issues in travis with secure environment variables not being available for PR builds to avoid them being vulnerable to malicious PRs.

That's the issue. I had Build pull request updates unticked before you submitted your earlier PR because I was paranoid about this. Now that it's ticked, it doesn't work because the env vars are not being exposed.

Do you know of a good solution other than not providing this functionality and instead pulling remote code + opening separate PR + closing the one submitted by contributor?

[lennym]

My only thought is possibly to build PRs with tests run "locally" against a phantomjs instance, and reserve a full saucelabs test for master builds.

If it's any use, I made a little script helper for running conditional travis builds a while ago, which we found to help with doing things like this - https://www.npmjs.com/package/travis-conditions. We actually use it to get around this exact issue - namely we have auth tokens for snyk, which are unavailable for PRs.

Edit: found the project in question: https://github.com/UKHomeOfficeForms/hof-bootstrap/blob/master/.travis.yml

[tvararu]

@lennym I thought about that, however, I had to move away from phantomjs because I found our integration tests to not produce relevant results on that browser. (using webdriverio, selenium, the respective drivers so it could be an issue from a number of parts not just phantomjs itself)

I'll open an issue to discuss this further but until then I will deactivate Build pull request updates.

I'll merge this, and publish a new version after master builds.

Thanks again!

[lennym]

I'd strongly recommend trying a conditional build so you can still run your linter and unit tests on PRs. At least that way you can automate most of the testing.

[tvararu]

Published in v1.0.2!

@lennym happy to chat more in #129, might have to get back to this at a later point.

[lennym]

I've been doing a bit more looking at this this morning and I'm now unsure that what I've done is correct.

Our desired behaviour (and IMO, what should probably be default behaviour) is something like this:

• If user hits the page for the first time and no value has previously been selected (technically that means the <option value="">Select</option> is selected) then they see a blank text box.
• If the user has previously entered a value, and another option is selected then they see the innerHTML of the selected <option>.

The problem with this is that it's quite tricky to "nicely" configure with the options as they are - you end up needing to add a condition to check if the value of the select exists, and then either set the defaultValue to be '' or the innerHTML accordingly.

I think a better approach is to incorporate this into the enhanceSelectElement function so that it only uses the defaultValue option if the "default" option is selected (i.e. selectElement.value == false) and uses the label of the selected element otherwise.

tl;dr - more PRs coming

---

Edit: by "nicely" configure, I mean that the options to do this look like this:

ac.enhanceSelectElement({
  defaultValue: elm.value ? elm.options[elm.options.selectedIndex].innerHTML : '',
  selectElement: elm
});

[tvararu]

@lennym yes, that does sound like a more sensible default. 👌

[lennym]

Super. I'm just finishing off #130 and then I'll implement roughly what I've described above.