-
Notifications
You must be signed in to change notification settings - Fork 31
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
Update example with 8 fields #65
Conversation
new field begin with no contents, and editing field begin with some content to be editied
Hey @CGKarnlund thanks for contributing again :), it's super cool to have your help. I saw that the CI failure is because it is building for iOS 8.0, which I don't know why, but I'll check. Just to let you know that it is not related to your changes. This links to #62 . |
It's probably getting errors because somehow some references to Swift 4.2 creeped into my checkin in the project files. I just noticed it earlier today. It may be a few days before I can clean that one up. We were using a branch I made that was compatible with Xcode10 and Swift 4.2. That's what was unknowingly causing the cursor problem that we were seeing. But we are now switching to Swift 5.0 and Xcode 11 and that seems to be more what you built the project one. |
9f3fee7
to
1a1037a
Compare
Because the changes are only on the Example app, I think you can commit only the Example app Storyboard and view controller changes. This should fix the problems. |
Regarding the Example App, IMHO it is looking more confusing than helpful after the changes. What do you think? |
I was just looking for a testing experience where seveal fields could be tested, while viewing them at the same time. So it's really fast and obvious to see when the experiences are not consistent. I certainly wouldn't mind if the example became more powerful letting other properties to be set too. |
Yep, I like your idea and I see value, but I think that unit testing different setups would be a better direction. It would work really well with the CI to prevent problems when changes are applied. |
So you are suggesting an iOS UI Testing target to confirm that the fields act consistently using CI test? That sounds like it could be awesome. |
Yes, I think both unit testing and UI testing. On unit tests, there are already a few test cases that send actions to text field delegate to validate its And yes, as complement a new UI testing target, with UI interactions would be great to make sure it works well as a whole when the user is really interacting with it. With launch arguments or/and test plan I think we would be able to run the black box testing with different locale and currency setups. |
It's just not my area of expertise to set that up. I believe that systems like this fairly simple delegate can generally be stabilized pretty quickly to work for everyone in the world without many changes required in the future to keep it working. It's most likely that the variability of Swift itself is the greatest factor in how long CurrencyText could run without needing changes to keep it going. So I think there is a strong argument that it might not really be worth that effort level, but maybe you've done it enough that it's fairly quick? The last time I saw tons of UI testing in like 2014, it turned out to be a massive headache of half working stuff and hacks. Any ideas what causes the hasDecimals issue that I've noted? Luckily our app doesn't use decimal places, so it's a hidden bug to us. |
@CGKarnlund Yes, UI tests are generally flaky. AFAIK there are techniques to run them as white box tests, but as you said for a fair simple project there is not a big need for it. That why I would prefer to stick with unit tests, the logic is quite simple, but it can be easily broke when changing something (e.g. by updating the cursor logic). Creating more unit tests cases for different setups and cases, can make sure things work well for everyone and that no regression is added. I also really like them as a tool to enforce a good code, such as when doing TDD. Anyway, I'll get back to them later (or anyone who wants to contribute) to improve the coverage. There is already an issue created to tackle this #63. Regarding your question, what is the issue with |
I don't think that what I reported above is related to ticket #64. I think it has to do with minValue = 1, and having hasDecimal = true. In those cases minValue should be .01, or the field will misbehave. This might be be a good condition to detect an illegal configuration and correct it. In all the fields that do not contain initial values
|
Ah I see. I remember about this now. I'll create a ticket for it as well, but not sure yet how to proceed. Feel free/more than welcome to give your thought there for this to be improved. Regarding this pull request, I'll close it, the example can be soon changed following that concept of guiding new users on how to use the framework. There's also another ticket/issue for adding more unit test cases. |
@CGKarnlund this is the created ticket |
New fields begin with no contents, and editing fields begin with some content to be edited.
Why?
Help us better see how the field reacts in multiple currencies and field configurations all at once.
Changes
Added 5 text fields in various configurations of initial values to just the example app.
Tests
Run the example app.
Issue (optional) add link to the issue here
All fields that do not contain initial values, but also have decimal values configured, where hasDecimal is set true, misbehave when the user types "1" then "2" in those fields.
In Euros the value result is "10,02 €". The expected result is "0,12 €".
In Dollars the value result is "$10.02". In Dollars the value result is "$0.12"
So far I haven't noticed any other problems with editing or cursor.