-
Notifications
You must be signed in to change notification settings - Fork 9
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
issue 12: fixed tooltip flicker; prevent tooltip run off screen #13
Conversation
… from running off the screen, in some cases
Hey thanks for raising the pr, will check this |
coachmark/src/main/java/com/pseudoankit/coachmark/overlay/DimOverlayEffect.kt
Outdated
Show resolved
Hide resolved
coachmark/src/main/java/com/pseudoankit/coachmark/overlay/OverlayLayout.kt
Outdated
Show resolved
Hide resolved
coachmark/src/main/java/com/pseudoankit/coachmark/overlay/OverlayLayout.kt
Show resolved
Hide resolved
coachmark/src/main/java/com/pseudoankit/coachmark/overlay/OverlayLayout.kt
Outdated
Show resolved
Hide resolved
import com.pseudoankit.coachmark.model.TooltipConfig | ||
|
||
/** Containing composable must use these values for layoutId on current and previous tooltip. */ | ||
public enum class OverlayChildLayoutId { CURRENT, PREVIOUS } |
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.
BTW isn't this layoutId for Tooltip, can we rename TooltipId
also do we need this as enum looks this can be a object and values as const
don't see any usage of having enums here
coachmark/src/main/java/com/pseudoankit/coachmark/overlay/OverlayLayout.kt
Outdated
Show resolved
Hide resolved
coachmark/src/main/java/com/pseudoankit/coachmark/overlay/OverlayLayout.kt
Show resolved
Hide resolved
coachmark/src/main/java/com/pseudoankit/coachmark/overlay/OverlayLayout.kt
Outdated
Show resolved
Hide resolved
coachmark/src/main/java/com/pseudoankit/coachmark/overlay/OverlayLayout.kt
Outdated
Show resolved
Hide resolved
I added a parameter gapTooltipScreen to control how close tooltips can approach left and right side of screen, but I didn't expose this in calling composables please expose it let's give client flexibility |
Somehow I can't reply here on the comment you made re: the enum. I replied on it in the code listing. |
coachmark/src/main/java/com/pseudoankit/coachmark/overlay/OverlayLayout.kt
Show resolved
Hide resolved
Thanks for the pr 🙏 , merged |
The heart of this change was implementing custom OverlayLayout to size and position tooltips without waiting an extra frame (if I understood the root cause correctly). I haven't witnessed any flicker in the test app, nor in my app, since this change.
I added more tooltips to the demo app. I can remove them if needed.
OverlayLayout: I added a parameter gapTooltipScreen to control how close tooltips can approach left and right side of screen, but I didn't expose this in calling composables. I can do so if you want.
It's still possible to specify tooltips so that they run off the top or bottom of the screen. This is a harder than preventing left and right "overrun"; I mentioned it briefly in the layout code.