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

"x" buttons have same id in HTML DOM #315

Closed
terrynguyen255 opened this issue Dec 17, 2019 · 8 comments · May be fixed by #325
Closed

"x" buttons have same id in HTML DOM #315

terrynguyen255 opened this issue Dec 17, 2019 · 8 comments · May be fixed by #325
Labels
stale wip Work In Progress

Comments

@terrynguyen255
Copy link

Describe the bug
"x" buttons have same id in HTML DOM.
The problem happens when every node has id property, when nodes have .id, "x" buttons stop using DropdownTreeSelect.props.id.

To Reproduce

  • Visit my codesandbox
  • Inspect the page, find for "button.tag-remove" (all buttons which's CSS class = "tag-remove")
  • Actual behavior: You will notice all found buttons have the same id="2_button"

Expected behavior
The buttons' ids should be different between different DropdownTreeSelects (like ${DropdownTreeSelect.props.id}_${node.id}_button).

image

image

@mrchief mrchief added the bug label Dec 18, 2019
@mrchief
Copy link
Collaborator

mrchief commented Dec 18, 2019

Yeah, this is a bug. In general, I think the id creation logic across all individual components will probably suffer from the same defect. A safer idea is to prefix with the root id itself.

A PR would be welcome!

@terrynguyen255
Copy link
Author

Yeah, I'd love to contribute to your awesome lib :>

mrchief added a commit that referenced this issue Jan 19, 2020
Currently, having multiple component instances on same page renders same remove-button, tag ids.

Fixes #315
@mrchief mrchief removed the bug label Jan 19, 2020
@mrchief
Copy link
Collaborator

mrchief commented Jan 19, 2020

@Nogias9x this is not a bug. If you check in your sandbox, you're assigning custom id to nodes in your data json. Since all instances are using the same demo data, they all get the same id.

I removed id nodes from your data and as you can see, you get unique ids across instances

image

The same goes for tags.

@terrynguyen255
Copy link
Author

Yes you are right @mrchief . But I think the .id should be reserved by the list items. For example, if the select is list of job positions, the .id should be position.id which is fetching from database.

@mrchief
Copy link
Collaborator

mrchief commented Jan 20, 2020

When you say it should be “xyz”, you’re unconsciously transitioning into business domain. To avoid being sandboxed into such decisions, the component lets you decide and shape that piece of information. E.g , you could assign the id to be that “xyz” that you need and the component will reflect that.

Another person could then use “abc” to be the id and the same component can serve both.

My PR is again a generic solution as we support the use case of having multiple instances and it makes sense semantically to have unique ids in the dom.

If you’re using id field to store some data, then you don’t need to do so as you can very well pass any arbitrary prop to node and it’ll bubble it back on change events.

@stale
Copy link

stale bot commented Feb 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 4, 2020
@mrchief mrchief added wip Work In Progress and removed stale labels Feb 4, 2020
@github-actions
Copy link

github-actions bot commented Dec 1, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Dec 1, 2021
@mrchief mrchief removed the stale label Dec 2, 2021
@github-actions
Copy link

github-actions bot commented Jan 1, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale wip Work In Progress
Projects
None yet
2 participants