Skip to content

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

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

seam0s-dev
Copy link

Added options on the frontend to selectively disable/enable certain types of overlays.

@Keavon Keavon force-pushed the master branch 3 times, most recently from aa7ff13 to e11b57a Compare April 6, 2025 11:41
@Keavon Keavon changed the title Creating Granular Settings on the Overlays Popover Add the settings popover menu for the Overlays toggle Apr 9, 2025
@seam0s-dev seam0s-dev marked this pull request as ready for review April 9, 2025 20:11
@seam0s-dev seam0s-dev marked this pull request as draft April 13, 2025 10:18
@seam0s-dev seam0s-dev closed this Apr 15, 2025
@seam0s-dev seam0s-dev force-pushed the granular-overlays-settings branch from c46f4b4 to 9a62c1c Compare April 15, 2025 20:26
@seam0s-dev seam0s-dev reopened this Apr 15, 2025
@seam0s-dev seam0s-dev closed this Apr 16, 2025
@seam0s-dev seam0s-dev force-pushed the granular-overlays-settings branch from f5b4e4c to 9a62c1c Compare April 16, 2025 11:05
@seam0s-dev seam0s-dev reopened this Apr 16, 2025
@seam0s-dev seam0s-dev force-pushed the granular-overlays-settings branch from 728e57d to 938ad07 Compare April 16, 2025 12:01
@seam0s-dev seam0s-dev force-pushed the granular-overlays-settings branch from 4477cc0 to c4c5379 Compare April 18, 2025 21:15
@seam0s-dev
Copy link
Author

@Keavon Is it good to open now?

@Keavon
Copy link
Member

Keavon commented Apr 18, 2025

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.

@seam0s-dev
Copy link
Author

seam0s-dev commented Apr 18, 2025

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.

@Keavon
Copy link
Member

Keavon commented Apr 18, 2025

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.

@Keavon
Copy link
Member

Keavon commented Apr 23, 2025

!build

Copy link

📦 Build Complete for 39449d3
https://66314e3c.graphite.pages.dev

// 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,
Copy link
Member

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.

Copy link
Member

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.

@Keavon
Copy link
Member

Keavon commented Apr 24, 2025

Bugs:

  • When Transform Cage is disabled, but dragging the compass rose by the X or Y arrows, we're incorrectly allowed to freely drag the object anywhere (not just along the constrained axis line).
  • When Transform Pivot is disabled, we're still able to drag the (invisible) pivot.
  • In the Pen tool, the handle lines are still drawn when handles are disabled— those should also be hidden. (But don't also go and hide the preview path when Path is disabled, we do want to keep that.)
  • Disabling Anchors shouldn't cause the existing selected anchors to become deselected like it currently does.

@Keavon Keavon marked this pull request as draft April 24, 2025 09:07
@seam0s-dev
Copy link
Author

Could you give a demo on what you meant by the bug with Anchors?

@Keavon
Copy link
Member

Keavon commented Apr 25, 2025

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.

@seam0s-dev
Copy link
Author

Just wanted to confirm. Thanks.

@seam0s-dev seam0s-dev force-pushed the granular-overlays-settings branch from 59ac1cd to b7fb453 Compare April 28, 2025 09:29
@seam0s-dev seam0s-dev marked this pull request as ready for review April 28, 2025 10:22
@seam0s-dev seam0s-dev force-pushed the granular-overlays-settings branch from 1719269 to 1634862 Compare April 28, 2025 11:45
@seam0s-dev seam0s-dev marked this pull request as draft April 28, 2025 11:56
@seam0s-dev seam0s-dev marked this pull request as ready for review April 29, 2025 14:21
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.

3 participants