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

Display RGB when hovering over images #778

Closed
wants to merge 1 commit into from
Closed

Display RGB when hovering over images #778

wants to merge 1 commit into from

Conversation

aashna27
Copy link

@aashna27 aashna27 commented Feb 15, 2019

Fixes #751

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
  • code is in uniquely-named feature branch and has no merge conflicts
  • PR is descriptively titled
  • ask @publiclab/reviewers and @publiclab/is-reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If 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!

@harshkhandeparkar
Copy link
Member

Alsk please provide a GIF

@aashna27
Copy link
Author

aashna27 commented Feb 15, 2019

Alsk please provide a GIF

Here:
ezgif com-video-to-gif 1

please review @publiclab/is-reviewers

Copy link
Member

@Divy123 Divy123 left a 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!!

Copy link
Member

@harshkhandeparkar harshkhandeparkar left a 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

@harshkhandeparkar
Copy link
Member

Also instead of showing the words rgb, can you fit in an alpha value?

@aashna27
Copy link
Author

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

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.

@aashna27
Copy link
Author

Also instead of showing the words rgb, can you fit in an alpha value?

yeah rgba can be added, I ll do that.

@Divy123
Copy link
Member

Divy123 commented Feb 15, 2019

@harshkhandeparkar I think adding RGB is better than alpha as it explicitly defines that pixel better.
Do you mean rgba or only the opacity?
Your words say the second thing.

@harshkhandeparkar
Copy link
Member

@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.

@harshkhandeparkar
Copy link
Member

harshkhandeparkar commented Feb 15, 2019

@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?

@Divy123
Copy link
Member

Divy123 commented Feb 15, 2019

Agreed @harshkhandeparkar I thought you were asking only for opacity.


var img = $(step.imgElement);

img.mousemove(function(e) {
Copy link
Member

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?

Copy link
Member

@harshkhandeparkar harshkhandeparkar left a 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;
Copy link
Author

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.

@aashna27
Copy link
Author

aashna27 commented Feb 15, 2019

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 ?

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.

ezgif com-video-to-gif 2

@aashna27
Copy link
Author

@jywarren

@jywarren
Copy link
Member

Rebasing this. @aashna27 I see you used debounce here -- we have a growing problem with typeahead autocompletion:

publiclab/plots2#3472

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?

@aashna27
Copy link
Author

@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!!

@jywarren
Copy link
Member

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. 👍

@jywarren
Copy link
Member

Hmm, what did I do wrong?

@jywarren jywarren mentioned this pull request Feb 21, 2019
4 tasks
@jywarren
Copy link
Member

Apologies! I just merged #803 -- i must've made some error here though. Do you think you could give it a try?

@aashna27
Copy link
Author

Apologies! I just merged #803 -- i must've made some error here though. Do you think you could give it a try?

No problem.. 😄

@aashna27
Copy link
Author

aashna27 commented Feb 21, 2019

I think I ll close this one , I have tried multiple times, its not building.Probably have to open a new one.

@aashna27 aashna27 mentioned this pull request Feb 21, 2019
4 tasks
@aashna27 aashna27 closed this Feb 21, 2019
@aashna27 aashna27 deleted the display-rgb branch March 16, 2019 11:22
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.

4 participants