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

[Button] data-attribute not returned through onClick event #3311

Closed
TrySpace opened this issue Feb 12, 2016 · 13 comments
Closed

[Button] data-attribute not returned through onClick event #3311

TrySpace opened this issue Feb 12, 2016 · 13 comments
Labels
component: button This is the name of the generic UI component, not the React module!

Comments

@TrySpace
Copy link
Contributor

I want to set a data-tag on MenuItem and access it through e.target.dataset.tag but it doesn't get passed through.

This works on vanilla elements.

Any idea how to get this working in another way? Or alternatively, would this be implemented in the future?

@mbrookes
Copy link
Member

Are you looking for ref by any chance?

@TrySpace
Copy link
Contributor Author

Not really, although that might work, and serve a similar purpose, I haven't tried it, but for vanilla elements it works and is more readable.

Main problem here is that the data-tag prop doesn't get propagated.

Question is, would you want all data-* to be set on all underlying elements, no, but I would want it to be on the elements I directly interact with, like the elems I onClick, or in the case of an inputfield in the onChange.

Because I believe components used should act as a singular instance to the outside user, without any knowledge of the other recycled modules that comprise it..

@Spyes
Copy link

Spyes commented Apr 25, 2016

Sorry to re-open this thread, but I need exactly this functionality. I have a data attribute that I need to pass to my onChange function, and I have no way of doing this... Any ideas? Thanks!

@TrySpace
Copy link
Contributor Author

@Spyes I think this would need to be implemented per Component in this repo

@Spyes
Copy link

Spyes commented Apr 25, 2016

@TrySpace For now I've modified material UI's ListItem component to also include the data I pass to it into the div, thanks

@baerrach
Copy link

The next branch will have the same issue as it doesn't spam the ...other props into all elements that get created or cloned.

My current work around is to lookup dataset in the parent hierarchy, as some element will have the correct data attribute populated, just maybe not the one you are expecting (or the one reported in event.target callbacks)

const datasetInHierarchy = (elem, attribute) => {
  if (elem.dataset && elem.dataset[attribute]) {
    return elem.dataset[attribute];
  }
  if (elem.parentElement) {
    return datasetInHierarchy(elem.parentElement, attribute);
  }
  return undefined;
};

@oliviertassinari
Copy link
Member

@baerrach Do you have a reproduction example with the v1-beta branch? Looking at the source code I would expect it to work but I could be wrong.

@oliviertassinari oliviertassinari added component: button This is the name of the generic UI component, not the React module! v1 waiting for 👍 Waiting for upvotes labels Jul 28, 2017
@oliviertassinari oliviertassinari changed the title [MenuItem] data-attribute not returned through onTouchTap event [Button] data-attribute not returned through onClick event Jul 28, 2017
@baerrach
Copy link

I'm not use next yet, I know I scanned through the code but I can see through the blame view that the code has been there for ages.
I don't know what I was looking at.

I can see:
ButtonBase passes all unknown props via ...other
ButtonBase renders ComponentProp which is either or , and unknown props via ...other are also passed to ComponentProp.

@oliviertassinari oliviertassinari removed the waiting for 👍 Waiting for upvotes label Jul 28, 2017
@oliviertassinari
Copy link
Member

Thanks. Let's assume it's fixed until we have a reproduction example.

@G-Rath
Copy link
Contributor

G-Rath commented Jul 18, 2018

@oliviertassinari what happened with this issue..?

The comments have me somewhat confused, as it sounds like it was fixed, but I don't see any evidence of it.

Currently we're using a button 'in the standard way', attempting to put data on it using data-*.
While the onClick event fires when the Buttons child elements are clicked, the target of the event is the element that was clicked, not the Button, meaning that event.target.dataset will not contain the Buttons dataset.

This isn't the end of the world, as we can specify the data-* attributes on the elements ourselves, since we're passing them in, like so:

<Button
    data-my-data="hello world"
    onClick={event => console.log(event.target.dataset)}
>
    <div data-my-data="hello world">Hello World</div>
</Button>

The problem arises when passing only text, like so:

<Button
    data-my-data="hello world"
    onClick={event => console.log(event.target.dataset)}
>
    Hello World
</Button>

With the above, if you click on the Hello World text, the console will output an empty DOMStringMap.

This is b/c the Button component wraps it's children in a span, and the target for onClick events on plain text is the parent node of that text (which in this case is the span, not the button).

The solution in my eyes would be to just pass all data attributes to the span as well.
Currently, I can't find anyway to access that span myself from outside the library.

Please let me know if I should create a new issue for this, if I've missed something, or if there's anything else I could do to help with this issue.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 18, 2018

The comments have me somewhat confused, as it sounds like it was fixed, but I don't see any evidence of it.

@G-Rath The status of the issue is: no reproduction example = no bug. So yes, it's supposed to be good.

I have a tip for you: event.target !== event.currentTarget.

@G-Rath
Copy link
Contributor

G-Rath commented Jul 23, 2018

@oliviertassinari sorry for taking so long to reply - only just had the time to check this out.

Thanks a ton - works perfectly! I knew there was a property for exactly that purpose, as I've used it in the past, but couldn't find it anywhere.

I suspect what threw me is that when inspecting the event in the console, currentTarget is always null for some reason, despite having called event.persist(). Lucky I know for next time.

Maybe it's worth dropping that line in the MUI docs somewhere, just as a couple of lines explaining how MUI handles currentTarget? (As I've seen a couple of messages around the repo about how MUI changes currentTarget in some situations, to try and align things with developer expectations).

@DavidHe1127
Copy link

e.currentTarget.dataset.* works like a charm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: button This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests

7 participants