-
Notifications
You must be signed in to change notification settings - Fork 24
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
Make hybrid annotations the default #4939
Conversation
…also don't show fallback options when no segmentation layer exists)
…dataset.isActive is true)
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.
Very cool, also the flow type refactoring 👍 I only left some very small suggestions regarding the code.
Regarding the UI, I agree with some of the points @fm3 already mentioned elsewhere and think there is some room for improvement. These are a couple of notes and ideas (feel free to ignore!):
- The dropdown is misleading as it's not recognizable that only the arrow triggers the dropdown. Initially, I thought the button needed to be clicked to trigger the dropdown, however, that started a hybrid annotation. Usually one would solve this by separating the arrow:
However, as we don't have the borders this doesn't really work 🤔 Sorry for not having a solution here, but I think we should try to find one. - A simple plus could work as an icon for the hybrid annotation
- While we're at it, a cog wheel icon would work better for the "Edit" option in my opinion and one could also name it "Settings" instead of "Edit"
- I think it would be fine to go with New Annotation directly instead of changing it again after a while, but no strong opinion
frontend/javascripts/oxalis/model/accessors/dataset_accessor.js
Outdated
Show resolved
Hide resolved
Thank you both for your feedback and ideas! I incorporated all the suggestions and regarding the dropdown/click target issue, I decided to make the entire "New Annotation" link a dropdown menu. One can directly click on it to create a new hybrid, but already hovering over it will show the dropdown with the other options. I think, this is the most idiomatic way and also antd's default menus support this. The fact that I had to manually tinker with the CSS before should have been a sign for me, that I did something weird 🙈 I updated the screenshot in the PR description (or check it out live). |
…nds/webknossos into refactor-new-annotation-links
Ah yes that’s also a fair idea :D |
Argh, of course you are right, point taken. My first thought was to add redundancy (so the first menu item would be equal to the link), but now I'm considering the following: What do you think? /edit: I used the |
Yeah definitely better 🤔 What do you think @daniel-wer? |
…ual menu link can also be clicked
I also thought about this but refuted the idea in my head with the argument @fm3 gave. However, I think you solved this nicely with the "Other options" divider. It's not perfect, but I think it's the best idea we have for now :) |
Hmm, now, that I slept over it, I got more hesitant. I see two downsides:
the second point is not super dramatic in my opinion, but in combination with the first, it makes me really doubt the approach.
I'll play around with that to see how this feels :) |
…actor-new-annotation-links
…New Annotation' link does not open a dropdown
Ok, I eventually went with your suggestion, @fm3. It currently looks like this: This solution doesn't put too much emphasis on the dropdown, the UI should be easy to understand and it also works on mobile. Seems like the right way to go :) |
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.
Great work @philippotto and thanks @fm3 for the testing and ideas!
To me this looks good to be merged 🎉
There is one minor issue but feel free to ignore - For some reason the vertical alignment of the 4 elements (icon + text + divider + arrow) irritates my brain, but I can't quite make out what's wrong:
Maybe the arrow is too low, or the divider too high? As I said, feel free to ignore 🙈
isActive === true
always means thatdataLayers
is an array. however, the back-end also needs to be adapted so that this is really true.I think, we can change the wording later to "New Annotation" (without "Hybrid"), but let's get the users accustomed to the new UI first.
URL of deployed dev instance (used for testing):
Steps to test:
Issues: