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

Attempt at listen fix #246

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Attempt at listen fix #246

wants to merge 1 commit into from

Conversation

awelles
Copy link
Contributor

@awelles awelles commented Jun 27, 2019

An attempt to de-clusterfy .listen,

while still allowing editing of input fields when(isActive)
but allowing setValue to cause updateDisplay, so events like keyboard events will work...

  • updateDisplay(force): skips active check (note force is false during the .___listening loop)

  • otherwise runs active check and doesnt update if dom.isActive(element)

  • fixes listen, while allowing editing, allows drag events, and re-added key up/down (which were blocked if checking isActive but not force)

isactive problems: fixes #206, fixes #179
keyboard: fixes #195, (which isactive checks block without upateDisplay(force) option)

possible related, but am confus:
#95: this is true. onChange is fired by setValue, onFInishChange only after slider/mouse event
#212: I don't follow this, though it seems related... a code example might help. I suspect he could updateDisplay(true) after whatever he's doing

withdrawing head from code in 3... 2.... 1....

checks dom.isactive & allows displayUpdate(force), set true for by default for setValue
fixes isactive problems
allows drag events, and re-added key up/down
@awelles
Copy link
Contributor Author

awelles commented Jun 27, 2019

trying to satify various issues orbiting __listening & updateDisplay.
Not sure I've done it, but test pass and I think the fixes I claimed above are truly fixed.

Can anyone tell me if #95 matters?
if #212 is not addressed?
if something else broke?

@jonathan-merlet-ocl
Copy link

The ColorController is affected by this problem as well ; I cloned your branch locally and modified it the same way you did for OptionController, and the problem is fixed.
Now, as I am new in contributing on github, I wonder what is the best way to proceed :

  • shoud I create a PR on your fix branch
  • or shoud I wait for your PR to be integrated in datgui and do a PR on my own on their master branch ?

@awelles
Copy link
Contributor Author

awelles commented Jul 24, 2019

The ColorController is affected by this problem as well ; I cloned your branch locally and modified it the same way you did for OptionController, and the problem is fixed.
Now, as I am new in contributing on github, I wonder what is the best way to proceed :

* shoud I create a PR on your fix branch

* or shoud I wait for your PR to be integrated in datgui and do a PR on my own on their master branch ?

Right! I forgot that one didn't I?
Push your changes to your github fork. Then you can PR me. Or you can just paste the change in a comment and Ill stick it in.
Or I'll just got fix it in a bit if you do neither. ty!

PS: I'd classify myself as a mostly-newbie here as well. Here's some resources that got me started:
https://guides.github.com/
https://guides.github.com/activities/forking/
https://reflectoring.io/github-fork-and-pull/

@donmccurdy donmccurdy added the bug label Apr 4, 2020
superkelvint added a commit to superkelvint/dat.gui that referenced this pull request Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants