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

Sibling w/ relative or absolute positioning can cause react-ink not to work #18

Open
ashtonsix opened this issue Jan 10, 2016 · 2 comments

Comments

@ashtonsix
Copy link

This also applies to children of siblings, and seems to depend on the order of elements. I believe the positioning messes w/ z-levels which means click events don't always propogate to react-ink.

Examples:
#1 Does work:

button([
  Ink()])

#2 Does work:

button([
  Ink(),
  span('text')])

#3 Doesn't work:

button([
  Ink(),
  span({style: {position: 'relative'}, 'text')])

#4 Doesn't work:

button([
  Ink(),
  div([
    span({style: {position: 'relative'}, 'text')])])

#5 Doesn't work:

button([
  Ink(),
  div([
    span({style: {position: 'relative'}, 'text'),
    span('text')])])

#6 Does work:

button([
  Ink(),
  div([
    span('text'),
    span({style: {position: 'relative'}, 'text')])])

#7 Does work (best workaround)

button([
  span({style: {position: 'relative'}, 'text'),
  Ink()])
@nhunzaker
Copy link
Contributor

Hmm. This is entirely because of relative/absolute positioning with z-index. The Ink component needs to catch the click/touch handlers in order to receive the signals to display the effect. If it isn't on top, that will not happen.

The easy fix would be to allow the Ink component to accept a style property (to use the second to last case as an example):

button([
  Ink({ style: { zIndex: 1 }),
  div([
    span('text'),
    span({style: {position: 'relative'}, 'text')])])

It looks like, presently, you can do this because we merge the style prop passed to Ink into the bare minimum style requirements for the component:

https://github.com/vigetlabs/react-ink/blob/master/src/index.jsx#L150

We should make it clearer in the readme about the z-indexing issues. Would you be willing to give your thoughts on this update once I'm able to write it up (hopefully later today)?

@ashtonsix
Copy link
Author

Sure thing

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

No branches or pull requests

2 participants