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

Update example with 8 fields #65

Closed
wants to merge 1 commit into from

Conversation

CGKarnlund
Copy link
Contributor

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.

new field begin with no contents, and editing field begin with some content to be editied
@marinofelipe
Copy link
Owner

Hey @CGKarnlund thanks for contributing again :), it's super cool to have your help.
I will take some time to review your changes in the next few days.

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 .

@CGKarnlund
Copy link
Contributor Author

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.

@marinofelipe
Copy link
Owner

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.

@marinofelipe
Copy link
Owner

marinofelipe commented Jan 29, 2020

Regarding the Example App, IMHO it is looking more confusing than helpful after the changes.
I think would be nice to provide a clear experience, where we can show how the formatting changes when different Currency formatting is applied.
I would personally like, for example, buttons (can be bar button items) to show a picker for currency, other for locale.. Maybe a modal screen to set hasDecimals, minValue, maxValue. I think it would bring more value for those that are trying to see the possibilities and how to set a formatter to match their expected output(s).

What do you think?

@CGKarnlund
Copy link
Contributor Author

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.

@marinofelipe
Copy link
Owner

marinofelipe commented Jan 29, 2020

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.
The project already has a good code coverage, but also covering other test scenarios using different configuration would be really beneficial.
I see the example in other hand as a playground for new users of the framework to try it out, see how it works and how to use it.

@CGKarnlund
Copy link
Contributor Author

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.

@marinofelipe
Copy link
Owner

marinofelipe commented Jan 29, 2020

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 text after different inputs. So there are ways to validate if the text is formatted as expected by mocking the behavior the user would have on the text field.
There I think even more test cases could be added, with e.g. different configurations, max set to z, min set to x.. and so on..

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.

@CGKarnlund
Copy link
Contributor Author

CGKarnlund commented Jan 29, 2020

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.

@marinofelipe
Copy link
Owner

@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 hasDecimals? For the other issue you reported, I've created this bug ticket #64. Is something related to it?

@CGKarnlund
Copy link
Contributor Author

CGKarnlund commented Feb 3, 2020

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
Where minValue=1
Where hasDecimal=true
The field misbehave when the user types "1" then "2" in those fields.

    public var hasDecimals: Bool {
        set {
            self.decimalDigits = newValue ? 2 : 0
            self.numberFormatter.alwaysShowsDecimalSeparator = newValue ? true : false

            // detect bad condition and correct it
            if let min = self.minValue, min > 0.01 {
                self.minValue = 0.01
            }
        }
        get { return decimalDigits != 0 }
    }

@marinofelipe
Copy link
Owner

marinofelipe commented Feb 3, 2020

Ah I see. I remember about this now. I'll create a ticket for it as well, but not sure yet how to proceed.
I think there are many ways to proceed with this, and I'm not sure if overriding the min value is the best way. I'll create a ticket then this can be handled separately there, considering different potential solutions.

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.

@marinofelipe
Copy link
Owner

@CGKarnlund this is the created ticket

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