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

Bug Fix: Prevent Card Fob from dispatching duplicate events when a fob is removed #5976

Merged
merged 2 commits into from
Oct 18, 2022

Conversation

rileyajones
Copy link
Contributor

Motivation for features / changes

#5968

Technical description of changes

The card fob would dispatch a timeSelectionChanged event on mouse up even if the time selection had not changed...

Detailed steps to verify changes work correctly (as executed by you)

  1. Start tensorboard
  2. Go to http://localhost:6006
  3. Enable step selection
  4. Open the console and clear it
  5. Dismiss the fob by clicking this button
    image

Alternate designs / implementations considered

Add a mousedown event listener to the dismiss button which prevents propagation. Unfortunately this would prevent clicks starting on the dismiss button from dragging the fob.

Copy link
Contributor

@JamesHollyer JamesHollyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird edge case. If someone drags the fob around then goes back to original spot we will not register the change. I guess this is ok. Maybe a boolean like "didMove" which flips on movement and resets when stopDrag is called?

@rileyajones
Copy link
Contributor Author

Weird edge case. If someone drags the fob around then goes back to original spot we will not register the change. I guess this is ok. Maybe a boolean like "didMove" which flips on movement and resets when stopDrag is called?

Do we actually want to fire a timeSelectionChanged event in that case though? In this case the time selection has not changed.

@JamesHollyer
Copy link
Contributor

Weird edge case. If someone drags the fob around then goes back to original spot we will not register the change. I guess this is ok. Maybe a boolean like "didMove" which flips on movement and resets when stopDrag is called?

Do we actually want to fire a timeSelectionChanged event in that case though? In this case the time selection has not changed.

Not a huge deal but, yea I think we would want to log the metric that they dragged the fob around. If the user drags the fob around...checks out some values and then moves it back to starting position I would want to track that in our metrics as a change.

@rileyajones
Copy link
Contributor Author

Weird edge case. If someone drags the fob around then goes back to original spot we will not register the change. I guess this is ok. Maybe a boolean like "didMove" which flips on movement and resets when stopDrag is called?

Do we actually want to fire a timeSelectionChanged event in that case though? In this case the time selection has not changed.

Not a huge deal but, yea I think we would want to log the metric that they dragged the fob around. If the user drags the fob around...checks out some values and then moves it back to starting position I would want to track that in our metrics as a change.

That makes sense, it should work that way now

@rileyajones rileyajones merged commit 26dec5b into tensorflow:master Oct 18, 2022
qihach64 pushed a commit to qihach64/tensorboard that referenced this pull request Dec 19, 2022
…b is removed (tensorflow#5976)

## Motivation for features / changes
tensorflow#5968

## Technical description of changes
The card fob would dispatch a `timeSelectionChanged` event on mouse up
even if the time selection had not changed...

## Detailed steps to verify changes work correctly (as executed by you)
1) Start tensorboard
2) Go to http://localhost:6006
3) Enable step selection
4) Open the console and clear it
5) Dismiss the fob by clicking this button

![image](https://user-images.githubusercontent.com/78179109/195714210-5f1bee4e-fb1b-4c89-ae24-e6933fa004c0.png)

## Alternate designs / implementations considered
Add a `mousedown` event listener to the dismiss button which prevents
propagation. Unfortunately this would prevent clicks starting on the
dismiss button from dragging the fob.
yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
…b is removed (tensorflow#5976)

## Motivation for features / changes
tensorflow#5968

## Technical description of changes
The card fob would dispatch a `timeSelectionChanged` event on mouse up
even if the time selection had not changed...

## Detailed steps to verify changes work correctly (as executed by you)
1) Start tensorboard
2) Go to http://localhost:6006
3) Enable step selection
4) Open the console and clear it
5) Dismiss the fob by clicking this button

![image](https://user-images.githubusercontent.com/78179109/195714210-5f1bee4e-fb1b-4c89-ae24-e6933fa004c0.png)

## Alternate designs / implementations considered
Add a `mousedown` event listener to the dismiss button which prevents
propagation. Unfortunately this would prevent clicks starting on the
dismiss button from dragging the fob.
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
…b is removed (tensorflow#5976)

## Motivation for features / changes
tensorflow#5968

## Technical description of changes
The card fob would dispatch a `timeSelectionChanged` event on mouse up
even if the time selection had not changed...

## Detailed steps to verify changes work correctly (as executed by you)
1) Start tensorboard
2) Go to http://localhost:6006
3) Enable step selection
4) Open the console and clear it
5) Dismiss the fob by clicking this button

![image](https://user-images.githubusercontent.com/78179109/195714210-5f1bee4e-fb1b-4c89-ae24-e6933fa004c0.png)

## Alternate designs / implementations considered
Add a `mousedown` event listener to the dismiss button which prevents
propagation. Unfortunately this would prevent clicks starting on the
dismiss button from dragging the fob.
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.

2 participants