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

Allow to disable automatic reloading of meshes during proofreading #7076

Merged
merged 9 commits into from
May 22, 2023

Conversation

knollengewaechs
Copy link
Contributor

@knollengewaechs knollengewaechs commented May 15, 2023

Steps to test:

  • See prerequisites in issue [Proofreading] Allow to disable automatic reload of mesh #6976
  • Activate proofreading mode in toolbar
  • If switch button is activated, meshes should render as before when clicking a segment
  • If it is deactivated, meshes should not be rendered after clicking a segment
  • Setting should be stored for a user, so logging on and off shouldn't change the setting of the switch

Issue:


@knollengewaechs knollengewaechs self-assigned this May 15, 2023
@knollengewaechs knollengewaechs marked this pull request as ready for review May 15, 2023 14:31
@philippotto
Copy link
Member

works great 👍 but design-wise, I would prefer a button which can be toggled. See my other PR where I did something similar in the toolbar:

https://github.com/scalableminds/webknossos/pull/7051/files#diff-8047990d97d5e406272b9cf99129c0c419d8eb14782a064a601b4375263f2eafR986

Also see this slack discussion about that decision if you are interested.

@knollengewaechs
Copy link
Contributor Author

knollengewaechs commented May 16, 2023

@philippotto I thought your code concerning the button styling from the mentioned PR was very useful. is there any way to incorporate it into my code while keeping you as the author?
Maybe I could delete the snippet and you could commit it to this branch?

Other than that I addressed your review thus far :)

@philippotto
Copy link
Member

Great 👍 Two small things:

  • the opacity: isAutomatic... ? 0.5 : 1, style is missing, I think. that way, the state of the button is even more prominent
  • please add className="without-icon-margin" to the button to avoid the small margin-right that is added by default to the icons. in my linked PR, there is the "AI" label next to the icon which is why it wasn't necessary there (for the clean-up button the className is also missing on the main branch currently, but I added it in my PR)

@philippotto I thought your code concerning the button styling from the mentioned PR was very useful. is there any way to incorporate it into my code while keeping you as the author?
Maybe I could delete the snippet and you could commit it to this branch?

That's very kind of you, but there's no need for it :) In the end, the commits of the PR are squashed anyway and you will be listed as the main author. Also, we don't really track statistics about commit authors, so it doesn't really matter in the end (:

@knollengewaechs
Copy link
Contributor Author

knollengewaechs commented May 16, 2023

I left the opacity out because it felt more like an adjustment if the button was disabled. I will add it back in :)
Edit: And now that I tried it out it doesnt feel that way any more

@philippotto philippotto changed the title Allow disable automatic reload of mesh Allow to disable automatic reloading of meshes during proofreading May 16, 2023
Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

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

Great, thank you :)

@knollengewaechs
Copy link
Contributor Author

@philippotto I felt like the merge conflicts were non-trivial. My new button is still working as expected, but could you check whether I destroyed any functionality of your auto-select SAM PR (or anything else)?

@philippotto
Copy link
Member

@philippotto I felt like the merge conflicts were non-trivial. My new button is still working as expected, but could you check whether I destroyed any functionality of your auto-select SAM PR (or anything else)?

The changes look good :) Feel free to merge!

@knollengewaechs knollengewaechs merged commit 08cd981 into master May 22, 2023
@knollengewaechs knollengewaechs deleted the allow-disable-automatic-reload-of-mesh branch May 22, 2023 09:26
hotzenklotz added a commit that referenced this pull request May 31, 2023
…ty-list-drawings

* 'master' of github.com:scalableminds/webknossos:
  [Docs] Update embedded YouTube URLs (#7102)
  Show organization in dataset info tab (#7087)
  Added Tutorials to Docs (#7095)
  Combine both download modals into one (#7068)
  Add "merge" blend mode (#6936)
  Changed the spacing/width of VX reports runs selection (#7094)
  Allow dataset managers to see all workflow reports of their orga (#7081)
  Fix rectangle at 0,0,0 (#7088)
  Allow to disable automatic reloading of meshes during proofreading (#7076)
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.

[Proofreading] Allow to disable automatic reload of mesh
2 participants