-
Notifications
You must be signed in to change notification settings - Fork 4
Add Configuration Builder (and tests) #146
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
base: trunk
Are you sure you want to change the base?
Conversation
786184c
to
77be22f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a high level, this makes sense to me. Solid improvements—thank you! Testing the Demo app worked as expected.
This is likely already understood, but building WP-iOS with these changes fails currently. Before merging this, we should open a sibling PR updating the WP-iOS implementation.
That said, I wanted to address the following:
The configuration has concerning mutability – it should never change after it's created because it's injected at the start. Once it starts being consumed, changes to it don't apply elsewhere, so by making it immutable it's harder to use it wrong.
It's not entirely true that mutations do not have an impact at the moment, because we explicitly rely upon mutating the configuration to set the editor settings and start the editor after the host app fetches the settings.
I am quite confident that code could/should be refactored to be less complex. There are a couple of nuances to consider:
- We "warm up" the editor on the My Site tab with an empty configuration;
- Within
init
, we initialize the editor configuration and view controller; - And within
viewDidLoad
, we display an activity indicator and fetch the editor settings.
These nuances could likely be refactored to embrace immutable configuration while retaining the necessary loading sequence.
@dcalhoun – I broadly agree with your comment. IMHO having two methods on the editor like I'm happy to leave this PR with you, feel free to do with it what you think is best! |
Apply iOS conditionals used in other, existing code.
This was changed while making the tests pass when running on macOS, but it is unclear why the original CI script was changed. Presumably we should run the tests with a fixed device on the CI. debc1c7#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L62
77be22f
to
26c3afa
Compare
@crazytonyli in addition to help with wordpress-mobile/WordPress-iOS#24662 (comment), I welcome your review of these changes now that I've rebased it atop the latest |
What?
Android uses a configuration builder which is much cleaner around mutability. This PR adds a configuration builder to iOS as well.
Why?
The configuration has concerning mutability – it should never change after it's created because it's injected at the start. Once it starts being consumed, changes to it don't apply elsewhere, so by making it immutable it's harder to use it wrong.
How?
Builder pattern
Testing Instructions
Covered by new unit tests.