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

Adds a visualizer to KIF #1179

Merged

Conversation

dostrander
Copy link
Contributor

Adds a touch visualizer to see where touches are being sent to. Currently it has a default look and feel. We can have a configuration object that can get set if we feel in the future we want customizability over the visualization of touches.

This is an explicit opt-in feature similar to printing the view hierarchy.

Copy link
Contributor

@justinseanmartin justinseanmartin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super stoked about this landing!

@dostrander dostrander force-pushed the dostrander/add-visualizer branch from c882003 to 8c5ec62 Compare September 15, 2020 20:09
Copy link
Contributor

@harleyjcooper harleyjcooper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really cool! One small comment, but it otherwise looks awesome.


@interface KIFEventVisualizer : NSObject

+ (instancetype)sharedVisualizer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to set - (instancetype) init to NS_UNAVAILABLE? Or would anyone else be calling the init method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good callout, since we are creating our own window overlay we probably want only 1 being created.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to make an initPrivate method because since it's marked unavailable using init internally also breaks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@dostrander dostrander force-pushed the dostrander/add-visualizer branch 2 times, most recently from 4c5a35e to 929dd5b Compare September 15, 2020 21:32

- (void)visualizeEvent:(nonnull UIEvent *)event;

// Unavaiable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*unavailable

@dostrander dostrander force-pushed the dostrander/add-visualizer branch from 929dd5b to 31999ee Compare September 15, 2020 22:00
@dostrander dostrander merged commit 71b3d69 into kif-framework:master Sep 16, 2020
@jgavris
Copy link

jgavris commented Sep 22, 2020

I don't believe the CocoaPods podspec includes these new files, we had been running KIF at HEAD for a while and had a build failure introduced with this PR. I see that the CI only tests Carthage. Do most people use Carthage these days?

@dostrander
Copy link
Contributor Author

@jgavris I'll fix that this morning, sorry about that. I don't think most use Carthage. Most probably aren't on HEAD though.

I'll also fix up the project so we catch this in CI (or earlier).

@jgavris
Copy link

jgavris commented Sep 22, 2020

@dostrander That would be excellent! Thanks. For our internal pods, we just have a really simple example project that imports the pod and checks that it builds.

@jgavris
Copy link

jgavris commented Sep 22, 2020

At the same time, for third-party libraries, we are starting to recommend not building from source, as those iOS build times grow and grow. So Carthage with something like Rome, or Cocoapods with the binary plugin, but that also has to lean on good CI in the external libraries.

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.

4 participants