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

Use event listeners to select a row of times #29

Open
wants to merge 14 commits into
base: gh-pages
Choose a base branch
from

Conversation

andytlr
Copy link
Owner

@andytlr andytlr commented Sep 6, 2014

Fixes #27

  • Get whole row selecting on click
  • Toggling of row selection
  • Don't let the current time be selectable.
  • Set var interval for the things that still use intervals to something reasonable. like 1000.
  • Turn application cache back on.

@andytlr
Copy link
Owner Author

andytlr commented Sep 6, 2014

hourNode.addEventListener("click", addSelectedClass, true);
hourNode.addEventListener("click", hideOrShowEmailButton, true);
hourNode.addEventListener("click", selectOtherCellsInRow(index, selectedIndex, hourNode), true);

selectOtherCellsInRow doesn't run on click. It's still happening on the interval timer. Where as addSelectedClass and hideOrShowEmailButton are both running instantly.

This is really obvious if I set the interval timer to two seconds:

delay

The email button is added instantly, and the clicked cell is highlighted instantly. But the row is only highlighted after a delay.

@samcambridge
Copy link

Are index and selectedIndex defined at this point?

@andytlr
Copy link
Owner Author

andytlr commented Sep 7, 2014

They should be. I had to pass them as arguments to make it work inside the function.

@samcambridge
Copy link

They may not be at the time the event listener is set up.

Add "debugger" (without the quotes, can't do markdown on my phone) above the line the event listener is on and reload the page with your chrome console open.

It should pause the script for you and you'll be able to type variable names in the console, if they return as undefined they're not set.

Alternately if they aren't set you probably would be getting JavaScript errors in your browser console.

@andytlr
Copy link
Owner Author

andytlr commented Sep 7, 2014

Thanks @samcambridge

selectedIndex = undefined; until this runs:

function addSelectedClass() {
  this.classList.add("selectedhourforsharing");
  selectedIndex = index;
}

Which is run by this on click:

hourNode.addEventListener("click", addSelectedClass, true);

Which is above:

hourNode.addEventListener("click", selectOtherCellsInRow(index, selectedIndex, hourNode), true);

So I thought by the time selectOtherCellsInRow ran, it would be aware of what selectedIndex was.

@samcambridge
Copy link

Everyone loves Race conditions!

@andytlr
Copy link
Owner Author

andytlr commented Sep 7, 2014

Ugh, right. Thanks.

@samcambridge
Copy link

Do you need an event listener for selectOtherCellsInRow or can you just call it directly from addSelectedClass?

@levibuzolic
Copy link

hourNode.addEventListener("click", selectOtherCellsInRow(index, selectedIndex, hourNode), true);

That doesn't run selectOtherCellsInRow on click, that will run the result of selectOtherCellsInRow on click. Which my guess isn't what you want.

When you write your event listener like that, selectOtherCellsInRow(index, selectedIndex, hourNode) is run once, immediately when the file is first parsed, and then never again. The return value of whatever that method is, will be what is run each time the event listener is called.

What I assume you want is this:

hourNode.addEventListener("click", function(){ selectOtherCellsInRow(index, selectedIndex, hourNode) }, true);

This will run the anonymous function every time the event listener is called, which will in turn run the selectOtherCellsInRow(index, selectedIndex, hourNode) method.

@levibuzolic
Copy link

To try and explain this a little better, take this example:

someFunction = function(){
  alert('Called');
};

node.addEventListener( 'click', someFunction );
// This will alert every time the node is clicked

node.addEventListener( 'click', someFunction() );
// This will alert once on file load, and never again even if we click the node

@levibuzolic
Copy link

One thing to note is that this may not fix your race condition, but race-condition-or-not with the way you're calling the function currently it's never going to work as expected.

@andytlr
Copy link
Owner Author

andytlr commented Sep 7, 2014

Thank you @levibuzolic that makes a lot more sense. Unfortunately I still haven't got it to work. I tried:

function selectOtherCellsInRow(index, selectedIndex, hourNode) {
  if (index == selectedIndex) {
    debugger
    hourNode.classList.add("selectedhourforsharing");
  } else {
    hourNode.classList.remove("selectedhourforsharing");
  }
}

hourNode.addEventListener("click", function(){ selectOtherCellsInRow(index, selectedIndex, hourNode) }, true);

If I click on the third row, in the debugger I get:

index
2
selectedIndex
2

Which is correct. The index is 2 and the selectedIndex is also 2. So I don't think it's a race condition.

It's like the function isn't aware of all hourNodes. Just the one I clicked on. If I take the if statement out of the function the debugger works quite differently. Inside the function you can see that it debugs the function and that's it. But if the if statement is outside the function it keeps looping through each hour because it's inside the forEach above:

[].forEach.call(hours, function(hourNode, index) {
...
}

@levibuzolic
Copy link

I'll try running the code locally tomorrow and take a proper look. There were a few things I saw earlier that could be tidied up, but I'll have to run it locally to get a better feel for what it's all doing. 👍

@andytlr
Copy link
Owner Author

andytlr commented Sep 7, 2014

❤️ thank you.

Conflicts:
	assets/app.js
	homeslice.appcache
@andytlr andytlr changed the title Use Event Listeners Use event listeners to select a row of times Sep 8, 2014
@samcambridge
Copy link

you still working on this mate? (finally came back to github :P)

@levibuzolic
Copy link

👋 HI SAM

@andytlr
Copy link
Owner Author

andytlr commented Nov 5, 2015

Oh hey guys

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.

Look into using event listeners instead of intervals
3 participants