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

clear button for Input & SearchInput #42

Merged
merged 5 commits into from
Nov 30, 2016
Merged

Conversation

eimfach
Copy link
Contributor

@eimfach eimfach commented Nov 30, 2016

This PR should resolve issue #32 .
Hello, this is my first contribution to an elm project. I did run the tests for chrome which passed (firefox seems not yet supported - missing geckodriver ? In my firefox tests mostly everything failed). Also, I wasn't sure which branch this PR to target for so I just used master.

The clear button works like that: it only appears if there is text and the input is not readonly or disabled.

Very good project structure, was very nice to work with. I would also try to resolve further issues.

Greetings

@gdotdesign
Copy link
Owner

Thanks for this PR, really appreciate it 😄

If you are willing to put some more work into it, I would suggest putting this feature under a flag in the model, something like showClearIcon : Bool and not make it show up by default.

There is no branching model at this point, so master is fine, I'm transitioning to use development but it's now contains the code for 0.5.0 with some breaking changes, after that we'll probably use it for PRs.

As for the tests, as you can see they are quite sparse and fail regularly for no reason, so I'm not really worried about them at this point. They should work with Firefox though, maybe the selenium package or your Firefox might be outdated, this would probably causes it to fail.

@eimfach
Copy link
Contributor Author

eimfach commented Nov 30, 2016

Thanks for your feedback, appreciate it too :)
I added the feature toggle you mentioned. Should it be able to be set on init ? As for now it just lives in the Model and needs to be enabled afterwards via update. I set the default to False.

@gdotdesign
Copy link
Owner

It's fine now, thanks :)

I'm moving away from setting thins in init, in 0.5.0 I'm adding functions for these kind of things so it would look something like this:

input = 
  Ui.Input.init "defaultValue"
    |> Ui.Input.placeholder "Some text..."
    |> Ui.Input.showClearIcon True

where the showClearIcon would be:

showClearIcon : Bool -> Model -> Model
showClearIcon value model =
  { model | showClearIcon = value }

If you like you can put it in there now, or I'll can do it later.

@gdotdesign gdotdesign merged commit d853f93 into gdotdesign:master Nov 30, 2016
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.

2 participants