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

Write UI test for "click to select coordinates" function #1846

Open
jywarren opened this issue Mar 12, 2021 · 13 comments
Open

Write UI test for "click to select coordinates" function #1846

jywarren opened this issue Mar 12, 2021 · 13 comments

Comments

@jywarren
Copy link
Member

#1187 introduced a nice new tool for selecting pixel coordinates by clicking the main image. However, we need a UI test to protect this, and I'd like to ensure we also have:

  1. a "crosshairs" icon next to the input: https://fontawesome.com/v4.7.0/icon/crosshairs (in the "addon" style of Bootstrap)
  2. activating this behavior only after you click on the corresponding coordinate inputs (so that people don't accidentally use it by clicking the image without knowing they're setting the coordinates)

We'd love help with this, building on top of the commits in #1187 so that we can incorporate that excellent addition as well! Thanks!

@jywarren
Copy link
Member Author

@vivek-30
Copy link

Ok i will try this😊

@vivek-30
Copy link

Actually @jywarren sir i went through the entire conversation held in #1187 and i think that i have already did this in #1804 . with all the requirements satisfied like -

  • having crosshairs icon in button
  • fill coordinates only when clicked on button.
    Now according to your issue statement the only thing left is Writing a UI test for this right sir ???
    Please correct me if i am wrong or missing something.

@vivek-30
Copy link

One more thing sir how do i push the code to the existing pull request won't there be any permission conflict there?.

@vivek-30
Copy link

@jywarren i have tried to write a test for this and everything is working fine except for only one thing. But don't know how to tackle that, could you please give some suggestions.

@vivek-30
Copy link

see -
Screenshot 2021-03-17 at 7 43 33 PM

the problem is when we select the image to click on it we have 2 images get selected and image.hover will hover on the first element it found hoverable.
see the documentation -
Screenshot 2021-03-17 at 7 50 49 PM

so how do we reach to second image ????

@vivek-30
Copy link

@jywarren sir after this the whole test will be completed

@jywarren
Copy link
Member Author

jywarren commented Apr 9, 2021

Hi, @vivek-30 my apologies for replying slowly. Just to catch up:

I think you're right that you've solved #1187 in #1804 however just to confirm i think you did it slightly differently - by defining a coordinate-input type rather than having an id in the form "id": "x-coord" as @aashna27 did?

If so we should be OK however i see that in #1187 @aashna also enabled this type of input for 4 modules:

  • CanvasResize
  • Colorbar
  • Crop
  • FisheyeGL
  • GridOverlay
  • Overlay
  • TextOverlay

So i think we have to add it to those too?

Finally, yes, the test is the last big piece. Let me look to see what to do...

@jywarren
Copy link
Member Author

jywarren commented Apr 9, 2021

For the test, are you working in a pull request you can link us to? So we can refer to specific lines of code?

I'm not sure which click event is getting the wrong image - i think we should be able to use more specific CSS to click but can you highlight:

  • which line is causing the issue
  • which image it's supposed to click
  • which image it's actually clicking

then hopefully we can disentangle it! Thanks!

@vivek-30
Copy link

Hi, @vivek-30 my apologies for replying slowly. Just to catch up:

I think you're right that you've solved #1187 in #1804 however just to confirm i think you did it slightly differently - by defining a coordinate-input type rather than having an id in the form "id": "x-coord" as @aashna27 did?

If so we should be OK however i see that in #1187 @aashna also enabled this type of input for 4 modules:

  • CanvasResize
  • Colorbar
  • Crop
  • FisheyeGL
  • GridOverlay
  • Overlay
  • TextOverlay

So i think we have to add it to those too?

Finally, yes, the test is the last big piece. Let me look to see what to do...

No Problem @jywarren sir , I understand that you have lot of work to do and finding time for all such things is very hard.
Taking about the issue, yes you are absolutely right i did it in a bit different manner as that issue is targeting only ease in adjusting text on the image. @aashna27 did it for #1187 which solely focuses on input-coordinates. No doubt that her approach is to the point and amazing . what we can do is just complete this test and then we can merge her PR. later i will modify the code for above constraints like (crosshairs icons , fill coordinates on click) and what i have added in #1804. thanks.

@vivek-30
Copy link

For the test, are you working in a pull request you can link us to? So we can refer to specific lines of code?

I'm not sure which click event is getting the wrong image - i think we should be able to use more specific CSS to click but can you highlight:

  • which line is causing the issue
  • which image it's supposed to click
  • which image it's actually clicking

then hopefully we can disentangle it! Thanks!

sorry sir if i have confused you . i was talking about the PR #1187 , i thought after completing the test i have to add the commit in this PR . so thats why i asked "how will i add a commit to someone else PR with appropriate authority".

@vivek-30
Copy link

BTW i will try this again with some different approach ☺️

@jywarren
Copy link
Member Author

Hi, sorry, i may still misunderstand, but if you can add the test in its own PR that's fine - in theory your own solution should fulfill it, right?

Merging @aashna27's code would be great, but we don't want 2 different systems, and your seems fine -- do you see an added benefit of also merging @aashna27's code?

I'm sorry I didn't catch that the two PRs overlapped. We were holding @aashna27's code until we had the right testing infrastructure, and I feel bad that we are considering not merging it now. That's on me - apologies to @aashna27 and yourself for the mixup and I appreciate both your work and time, both of you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants