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

Fixup incorrect implementation of methods in Dom #4425

Closed
1 of 12 tasks
fenilgandhi opened this issue Mar 26, 2020 · 9 comments
Closed
1 of 12 tasks

Fixup incorrect implementation of methods in Dom #4425

fenilgandhi opened this issue Mar 26, 2020 · 9 comments
Labels

Comments

@fenilgandhi
Copy link
Contributor

fenilgandhi commented Mar 26, 2020

Most appropriate sub-area of p5.js?

  • Color
  • Core/Environment/Rendering
  • Data
  • Dom
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • Other (specify if possible)

Details about the bug:

While writing tests for methods in dom module, i noticed more than a few methods have missing edge case handling, incorrect handling of optional arguments or improper documentation of features. So i am creating this generalised issue for dom module. With each method that i update from now on, i will comment about what's wrong and why it needed to be fixed.

  • p5.js version: 1.0.0
  • Web browser and version: Chrome 80.0.3987.122
  • Operating System: Manjaro 19.0.2 Kyria (Linux 4.19.108-1-MANJARO)
  • Steps to reproduce this: -
@fenilgandhi
Copy link
Contributor Author

createInput - the arguments are not correctly parsed. If the user wants to set any falsy value to the input, they will not be able to do so.

@fenilgandhi
Copy link
Contributor Author

fenilgandhi commented Mar 26, 2020

createRadio

  1. The documentation is not matching with the implementation. The documentations say one param divId is allowed, while implementation reads existing_radios.
  2. Also divId represents id of id and input respectively, so there should be two params or an array but only a string is mentioned.
  3. The implementation has an loop of count,prev whose relevance i am not able to figure out.
  4. The same code is replicated for selected and value methods.
    @lmccart , @limzykenneth, i would like to re-implement it from scratch.Can i get a spec for what should be the ideal input params for this method ?

@fenilgandhi
Copy link
Contributor Author

createFileInput - It takes 2 input params, a callback and a multiple both of which are optional. The current implementation would not allow the method to be called with only multiple=true and no value of callback.

@limzykenneth
Copy link
Member

createInput - should be pretty straightforward to fix and not a big change so you can probably just go ahead with it.

createRadio - the functionality of createRadio() was changed at some point but the documentation did not change to reflect that. The intended functionality should be similar to createSelect() I think so if you want to reimplement it, try and copy the pattern of createSelect() while also changing the documentation.

createFileInput - In this case I think there's little reason for the callback to be optional so it may just be a documentation fix of making the callback a required argument.

@fenilgandhi
Copy link
Contributor Author

Thanks @limzykenneth. i will get on with the required changes.

@carlesgutierrez
Copy link

Hi,
I'm getting errors using select "p5js select reference"
Any relation to this?
erros:

Uncaught DOMException: /reference/#/p5/select:1
Failed to execute 'registerProcessor' on 'AudioWorkletGlobalScope': A class with name:'recorder-processor' is already registered.
at blob

@limzykenneth
Copy link
Member

@carlesgutierrez Doesn't seem like it. Looking at the error message it seems to be more due to some audio issue. Do you see the same error on other pages of the reference or only on the page for select()? May be better to open a new issue for this as I don't think it's related.

@fenilgandhi
Copy link
Contributor Author

fenilgandhi commented Mar 28, 2020

Yes, even i have seen that issue on a lot of pages.
@carlesgutierrez It's totally unrelated to this issue but do create an issue. And, if you are willing to, we would be happy to entertain a PR for it. Otherwise, I would try to fix it, once i am finish my current task in hand.

@lmccart
Copy link
Member

lmccart commented Apr 24, 2020

I believe this has been resolved now.

@lmccart lmccart closed this as completed Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants