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

Fixes #5207 #5495

Merged
merged 5 commits into from
Jul 29, 2022
Merged

Fixes #5207 #5495

merged 5 commits into from
Jul 29, 2022

Conversation

awelles
Copy link
Contributor

@awelles awelles commented Dec 7, 2021

Resolves #5207

Changes:
in src/dom/dom.js:

Altered p5.prototype.createRadio() to check for type p5.Element argument. Followed existing p5.prototype.createSelect logic.

p5.prototype.createRadio = function() {
  ...
  let self;
  ...
  if (
    arg0 instanceof p5.Element &&
    (arg0.elt instanceof HTMLDivElement || arg0.elt instanceof HTMLSpanElement)
  ) {
  ...
  } else if (
    // If existing radio Element is provided as argument 0
    arg0 instanceof HTMLDivElement ||
    arg0 instanceof HTMLSpanElement
  ) {
  ...

PR Checklist

  • npm run lint passes
  • npm run grunt passes

@welcome
Copy link

welcome bot commented Dec 7, 2021

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

@limzykenneth
Copy link
Member

@awelles Changes for #5497 has been mixed in here, probably by pushing them to the same branch. Please revert those changes here and possibly create a new branch for the other changes. Thanks!

@awelles
Copy link
Contributor Author

awelles commented Dec 17, 2021

@limzykenneth

Indeed I cross-pollinated there.

It's reverted, but if you'd like I could reclone p5.js and resubmit the pull without the extra commits.

@awelles
Copy link
Contributor Author

awelles commented Dec 18, 2021

Let me just take a second to express some misgivings I have with the code / my fix.

select() ends up calling _wrapElement(), which ends up calling createRadio() (or createSelect in a different case). Like this:

p5.prototype.select = function(e, p) { 
  ...
  return this._wrapElement(res);
  ...
}

p5.prototype._wrapElement = function(elt) {
... # checks elt.tagName and elt.children conditions
    # usually returns new p5.Element(elt, this)
    # but in our case:
    return this.createRadio(new p5.Element(elt, this));
...

Our problem is createRadio() has no idea what to do with a p5.Element.

I changed createRadio() to mimic the behaviour of createSelect() and things seem peachy. (if not passed a p5.Element everything should run as it always has.

But why is _wrapElement() calling createRadio()? Because the name _wrapElement is misleading and some other process expects it to be appending new elements to the DOM?

Assuming so, my fix modifies createRadio() and leaves _wrapElement() alone. Despite my itch to have _wrapElement() just return new p5.Element(elt, this);

That is all.

/end

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

Successfully merging this pull request may close these issues.

Unexpected behaviour with select method
2 participants