-
-
Notifications
You must be signed in to change notification settings - Fork 577
Add the settings popover menu for the Overlays toggle #2523
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
base: master
Are you sure you want to change the base?
Add the settings popover menu for the Overlays toggle #2523
Conversation
aa7ff13
to
e11b57a
Compare
c46f4b4
to
9a62c1c
Compare
f5b4e4c
to
9a62c1c
Compare
728e57d
to
938ad07
Compare
…ed selection outline check
4477cc0
to
c4c5379
Compare
@Keavon Is it good to open now? |
You'll need to first resolve the merge conflict. Then I can give it a test if you think it's ready for production, excepting for any feedback I find when testing that you didn't notice in your own (hopefully thorough) testing. |
At this point, I think it would be better to get you in the loop for review. More than bugs, I found out that I have added unintentional features that deviated away from your intentions. |
Ok, in that case, please mark it as ready for review with a comment about that (which you just gave) so it turns from gray to green and I can look at it in my review queue. |
!build |
|
// Types of overlays used by DocumentMessage to enable/disable select group of overlays in the frontend | ||
#[derive(PartialEq, Clone, Debug, serde::Serialize, serde::Deserialize, specta::Type)] | ||
pub enum OverlaysType { | ||
All, |
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.
You can probably remove All
and replace the whole OverlaysType
with Option<OverlaysType>
where None
is equivalent to All
being false and Some
is equivalent to 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.
Upon further thought, that's not true because you'll lose your settings when it becomes None
. I recommend just storing a separate bool outside of OverlaysType
.
Bugs:
|
Could you give a demo on what you meant by the bug with Anchors? |
Sure: if you have some anchors selected with the Path tool active, turning off then back on the anchors visibility causes them to all get deselected. |
Just wanted to confirm. Thanks. |
59ac1cd
to
b7fb453
Compare
1719269
to
1634862
Compare
Added options on the frontend to selectively disable/enable certain types of overlays.