search

Skyscanner / SkyFloatingLabelTextField

A beautiful and flexible text field control implementation of "Float Label Pattern". Written in Swift.
Apache License 2.0
4.09k stars 540 forks source link

Customize errorMessage position #271

Closed ghost closed 4 years ago

ghost commented 5 years ago

Error messages can be set above or below textfield

jaredegan commented 5 years ago

What do you think @k0nserv?

jaredegan commented 5 years ago

Actually, the new public/open vars are missing documentation blocks as comments.

jaredegan commented 5 years ago

The textColor of errorLabel does not get updated in updateColors.

spencerdiniz commented 5 years ago

The textColor of errorLabel does not get updated in updateColors.

Is this necessary? errorLabel.textColor is always errorColor, which is set in createErrorLabel().

spencerdiniz commented 5 years ago

Issues addressed and branch updated.

jaredegan commented 5 years ago

It is necessary. If a user of this class changes the errorColor after initialization, the color of errorLabel will not be updated and it will still be red.

spencerdiniz commented 5 years ago

It is necessary. If a user of this class changes the errorColor after initialization, the color of errorLabel will not be updated and it will still be red.

Fixed and pushed.

k0nserv commented 5 years ago

Hey 👋 I am a bit concerned about feature creep. There's more and more PRs that add custom variations and I am concerned that all these variants will decrease maintainability and increase the risk of introducing bugs. Because of this I'd ask if we can consider whether this can be implemented as a subclass rather than a variant

spencerdiniz commented 5 years ago

Hi. We did consider implementing this as a subclass, but that would require us to refactor a lot of our existing code that uses the current class. That’s why we decided to include the feature in the class itself and not subclass.

Also, when we considered adding this feature through subclassing, we found out the some of the properties and methods are private, which doesn't allow us to override them.

Bernix01 commented 5 years ago

any update on this?

odanu commented 5 years ago

Yeap, indeed guys, are these commits going to join the main branch? :)

ghost commented 5 years ago

@k0nserv and @jaredegan can we approve this PR to master? I understand the manutenability concern but the unit tests can help us to keep code working. Besides that, this specific feature is not that complex, it is about the error message position.

jaredegan commented 5 years ago

I am not an official maintainer. If the code is legit but the official maintainer doesn't want it, I don't know what the best path forward would be. For me I forked it and I am using my fork. A bit of a hassle to stay up-to-date with changes as that becomes a very manual process, but open source software is hard!

k0nserv commented 5 years ago

Since there seems to be a signficant number of people who are interested in this feature we should probably merge it. @rethink-eder can you add this behaviour to the example app please?

ghost commented 5 years ago

@k0nserv I just committed a change in the screen "Setting text properties". We added a switch that will change the errorMessage position.

adrianod1as commented 5 years ago

+1

ghost commented 5 years ago

Hi @k0nserv did you have the opportunity to check the last commit with the screen example of this new property?

ghost commented 5 years ago

Hi @k0nserv . Would you approve this PR?

alexbtlv commented 5 years ago

+1

CaioSanchez commented 5 years ago

Any update on this?

olegdanu-newstore commented 4 years ago

Hey people :) What is the status of this PR? What are the blockers to proceed on merging it? What should be done to make this PR mergeable?

ghost commented 4 years ago

Hi @k0nserv. Are we ready for approve this PR?

ghost commented 4 years ago

Hi @k0nserv. Are we ready for approve this PR?

k0nserv commented 4 years ago

The build is failing atm so that needs to be fixed at the very least

ghost commented 4 years ago

Hi @k0nserv. The issue is now fixed. We merged from master and fixed the errors. Build is now working. Can you approve this PR?