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

Warn if outside of bounding box #4544

Merged
merged 12 commits into from
Apr 30, 2020
Merged

Warn if outside of bounding box #4544

merged 12 commits into from
Apr 30, 2020

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Apr 13, 2020

This PR adds a warning to the position view in tracings if the position is out of bounds.
Additionally this PR resolves issue #4336 by adding the error message to the ignored list.
The bug from #4472 is also fixed here.

URL of deployed dev instance (used for testing):

Steps to test:

  • Open a dataset in view mode and move outside the dataset bounding box. The position view should turn orange and a tooltip should indicate that the user is outside the bounding box.
    • also check arbitrary views: Here the rotation view should also turn orange if the user is outside of the bounding box.
  • Repeat the same steps while doing any tracing task with a lower bounding box that the bounding box of the dataset. Once you leave the task bounding box the warning should appear again.

Issues:


@MichaelBuessemeyer MichaelBuessemeyer self-assigned this Apr 13, 2020
@MichaelBuessemeyer MichaelBuessemeyer changed the title Warn if outside of bbox Warn if outside of bounding box Apr 13, 2020
@philippotto
Copy link
Member

philippotto commented Apr 14, 2020

Cool stuff :) Was curious about the "out of bounding box feature" and had to check it out. I've got some visual feedback:

I find the orange background a bit too much. I'd change it from:

image

to:
image

I used the followng css style for the pin-icon div and the position string div:

color: rgb(255, 155, 85);
border-color: rgb(241, 122, 39);

Also, I'd change the tooltip text to "The current position is outside of the dataset's bounding box. No data will be shown here."

@MichaelBuessemeyer
Copy link
Contributor Author

Thanks for your feedback @philippotto.

"The current position is outside of the dataset's bounding box. No data will be shown here."

I think this message is not fitting if the user is outside of the task bounding box. Should I adjust the message in that case? e.g.:
"The current position is outside of the task's bounding box. No annotation is possible here."

@MichaelBuessemeyer
Copy link
Contributor Author

I just tried annotation outside of the task bounding box and noticed that this is possible. Thus I'll change

"The current position is outside of the task's bounding box. No annotation is possible here."

to

"The current position is outside of the task's bounding box."

@MichaelBuessemeyer
Copy link
Contributor Author

@daniel-wer @philippotto One question I got left is: Shall the rotation view also be colored in the warning color and be included in the container that has the error tooltip?
Here is an image of the current state:
position-error-message

@daniel-wer
Copy link
Member

@MichaelBuessemeyer If it's easy I would only apply the style to the position box as the warning refers to the user's position.

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Lots of great fixes! I added some comments and would like to talk and understand the solution for the TD-View rotation center change :)

@MichaelBuessemeyer
Copy link
Contributor Author

@daniel-wer I just implemented your last feedback (see my comment). I can confirm that the fix still works on my laptop.

Could you please check this PR again?

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Awesome fixes, LGTM 🎉

@bulldozer-boy bulldozer-boy bot merged commit 6f95f92 into master Apr 30, 2020
@bulldozer-boy bulldozer-boy bot deleted the warn-if-outside-of-bbox branch April 30, 2020 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment