-
Notifications
You must be signed in to change notification settings - Fork 209
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
Display RGB when hovering over images #778
Conversation
Alsk please provide a GIF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is ready to go.
Thanks!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I also have a (partly unnecessary) feature request. I don't know if this does it already. But does it show the value only if the user hovers for a specific amount of time. Like if a mouse enters the image and moves then nothing happens but if the mouse is steady for let's say 2s then the values are shown
Also instead of showing the words rgb, can you fit in an alpha value? |
yeah it does expect a steady wait, Actually this is with the tooltip, if I just display the results normally then it's amazingly quick, but with tooltip a little steadiness is needed. |
yeah rgba can be added, I ll do that. |
@harshkhandeparkar I think adding RGB is better than alpha as it explicitly defines that pixel better. |
@aashna27 one last thing. I don't know if this is necessary but please check if the values are right by maybe using an image which is half one color and half another color. |
@Divy123 this feature will mostly be used for debugging (and geeks?) I guess. So adding an alpha can help in debugging. What do you think? |
Agreed @harshkhandeparkar I thought you were asking only for opacity. |
|
||
var img = $(step.imgElement); | ||
|
||
img.mousemove(function(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused. Why is the mousemove event handler added again here if it is already added above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about drawing the canvas during onComplete
step UI method? All that has to be done after that is update the tooltip. Also there will be no need to use debounce since the function will not slow down the browser that significantly. Also can the debounce cause the mouse move events to get out of sync and display wrong values since the mouse can move after an event was fired but the debounce function was not fired ?
|
||
var offset = $(this).offset(); | ||
var xPos = e.pageX - offset.left; | ||
var yPos = e.pageY - offset.top; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this required an event for the position and that's why I added "mousemove" here and yess I did do it for onComplete and it works but I thought that instead on adding the code there and increasing it lets make it modular so I made a separate function.
Actually it can slow, cause although the tooltip is slow but the computations occur really fast, when I console.log() the values it was really fast so I added debounce.And for the out of sync I think the steadiness of the tooltip will help to not get out of sync. |
Rebasing this. @aashna27 I see you used debounce here -- we have a growing problem with typeahead autocompletion: I'm wondering if you'd be able to help us out as this becomes more urgent. I think using debounce and/or other JavaScript throttling strategies can be part of the solution. Would you be willing to help out a bit? |
@jywarren thanks for the rebase, and yess I would have a look at it, but since I am completely unfamiliar with the codebase of plots2 I would take time, but yeah I will have a look at it. Thanks for considering me!! |
I believe there are code links to the necessary JavaScript segments -- thanks, we could use all the help we can get! Just pushed it up here. 👍 |
Hmm, what did I do wrong? |
Apologies! I just merged #803 -- i must've made some error here though. Do you think you could give it a try? |
No problem.. 😄 |
I think I ll close this one , I have tried multiple times, its not building.Probably have to open a new one. |
Fixes #751
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm test
@publiclab/reviewers
and@publiclab/is-reviewers
for help, in a comment belowIf tests do fail, click on the red
X
to learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Thanks!