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

CoachMark misplaced on iPad #160

Closed
o15a3d4l11s2 opened this issue Apr 11, 2018 · 11 comments
Closed

CoachMark misplaced on iPad #160

o15a3d4l11s2 opened this issue Apr 11, 2018 · 11 comments

Comments

@o15a3d4l11s2
Copy link

The position of the shown CoachMark is incorrect for iPad (both portrait and landscape) is not correct. The arrow is shown at the centre, where it should be, but the dialog itself is dispositioned:
simulator screen shot - ipad pro 10 5-inch - 2018-04-11 at 16 33 57

You can use the attached demo project for reproduction.
Demo.zip

@o15a3d4l11s2
Copy link
Author

@ephread , any information on this issue? Is there something more that I can do to help you trace the issue and fix it?

@ephread
Copy link
Owner

ephread commented May 27, 2018

Hey @o15a3d4l11s2, thanks a lot for providing a sample, that's really helpful!

Sorry about that. The issue is that the layout engine is a bit dumb. It doesn't center the coach mark body around the arrow (more on that later). Rather, depending on the horizontal position of the pointOfInterest, the engine chooses a fixed place for the body (left, middle, right – it's a bit like text alignment). Usually, it works fine, because the size of the content will eventually make the body grows enough to reach the arrow back. But in this particular occurrence, it doesn't.

Instructions was originally designed for the iPhone and I obviously never bother to test for all possible case on the iPad (nor did I even think of all these cases). Early on, I didn't want to pre-compute the width of the content and laying out in two passes, but I now believe that it wasn't that much of a good decision.

To be honest, there are a number of tiny issues in the layout engine that have been bugging me for a while. So, I'm going to rewrite the engine, but I can't really give you a time window as to when it's going to be available.

@o15a3d4l11s2
Copy link
Author

o15a3d4l11s2 commented May 27, 2018

A proposal - I know it is not optimal, but what about adding a "fake" view with size 1x1, which is added on the pointOfInterest coordinates?

This way you can set constraints related to this "fake" element and centre the coachMark on it. Adding horizontal constraints to the superview will ensure that the coachMark will not go outside the container and therefore, the screen.

An example of my proposal:

func constraints(for coachMarkView: CoachMarkView, coachMark: CoachMark, parentView: UIView,
                 layoutDirection: UIUserInterfaceLayoutDirection? = nil) -> [NSLayoutConstraint] {
    if coachMarkView.superview != parentView {
        print("coachMarkView was not added to parentView, returned constraints will be empty")
        return []
    }

    if layoutDirection == nil {
        if #available(iOS 9, *) {
            self.layoutDirection = UIView.userInterfaceLayoutDirection(
                for: parentView.semanticContentAttribute)
        }
    } else {
        self.layoutDirection = layoutDirection!
    }

    let microView = UIView(frame: CGRect(origin: coachMark.pointOfInterest!, size: CGSize(width: 1, height: 1)))
    coachMarkView.superview?.addSubview(microView)
    
    var constraints = NSLayoutConstraint.constraints(withVisualFormat: "H:|-(>=0)-[coachMarkView]-(>=0)-|", options: [], metrics: nil, views: ["coachMarkView": coachMarkView])
    let horizontalCenterConstraint = NSLayoutConstraint(item: coachMarkView, attribute: NSLayoutAttribute.centerX, relatedBy: NSLayoutRelation.equal, toItem: microView, attribute: NSLayoutAttribute.centerX, multiplier: 1, constant: 0)
    constraints.append(horizontalCenterConstraint)
    
    return constraints
}

@ephread
Copy link
Owner

ephread commented May 29, 2018

That's an option, but I believe it'll break for points of interest that are very close to the edge of the screen. Unless you have really short text (in the case of text-only coach marks), you won't able to center the content around the arrow without ending up with a very short width and insane word/character wrapping. And I'm not certain constraint priorities would help (I haven't tested them with the latest SDK, but I did a while ago).

I've posted a very long comment related to this matter on #165 if you wish to contribute.

@ephread
Copy link
Owner

ephread commented Jun 6, 2018

@o15a3d4l11s2 the issue should be fixed in master, let me know if it didn't do the trick! I've implemented the two-pass mechanism discussed in #165.

@o15a3d4l11s2
Copy link
Author

I checked both my real project and the demo project provided here and all works as expected.
When I initially tested the fixes there was an issue, but I noticed that you have made another fix since then and now everything is perfect.

Thank you for the library and the invested time and efforts!

@ephread
Copy link
Owner

ephread commented Jul 2, 2018

@o15a3d4l11s2 Great, thanks for the kind words!

@o15a3d4l11s2
Copy link
Author

@ephread , I could not find this included in a release. Could you please verify if it is released?

@vivek4292
Copy link

Hi @ephread,
I'm facing the same issue of misplacement of coach mark on iPad. I'm using this library version 1.2.0 via Carthage. Please let me know where I can find the fixes for this issue.

Thanks

@o15a3d4l11s2
Copy link
Author

o15a3d4l11s2 commented Nov 16, 2018

@vivek4292, as I see the fix it not merged into any released version. I was hoping to get response from @ephread, who was very kind to do the fix and I tested it, but unfortunately it seems he is quite busy at the moment. Hope to get the issue resolved soon.

@vivek4292
Copy link

@vivek4292 , as I see the fix it not merged into any released version. I was hoping to get response from @ephread, who was very kind to do the fix and I tested it, but unfortunately it seems he is quite busy at the moment. Hope to get the issue resolved soon.

Thanks, @o15a3d4l11s2 for letting me know. I Hope @ephread will respond us soon.

Tonbouy pushed a commit to Tonbouy/Instructions that referenced this issue Nov 29, 2019
Tonbouy pushed a commit to Tonbouy/Instructions that referenced this issue Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants