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

feat: Add option to flip single element on the context menu #2520

Merged
merged 13 commits into from Mar 26, 2021

Conversation

rileyschnee
Copy link
Contributor

@rileyschnee rileyschnee commented Dec 12, 2020

Preview: https://excalidraw-git-feature-mirrorselectionbutton.excalidraw.vercel.app

Addresses part of feature request #970

Summary of Changes:

  • created a new file src/action/actionFlip.ts and hooked it up to the context menu (based this off of actionAlign.ts)
  • vertical flipping relies on horizontal flipping, since it flips horizontally and then rotates by 180 degrees
  • exported a couple of functions that streamlined the flipping code to avoid redundancy
    Note: Text elements are the only ones treated differently. They can be flipped vertically (rotated 180), but not flipped horizontally.

Next Steps / PRs:

  • flipping groups of elements and retaining their relative placement to one another

Fixes #970

@vercel
Copy link

vercel bot commented Dec 12, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/8uYQmfjsjCYsQXr4rrjHQmx8YhVw
✅ Preview: https://excalidraw-git-fork-rileyschnee-feature-mirrorselect-032550.vercel.app

@dwelle
Copy link
Member

dwelle commented Dec 12, 2020

Works great already ❤️

image

@rileyschnee
Copy link
Contributor Author

I have added tests for flip. Please let me know if I'm missing anything. Since this is my first PR to excalidraw, I'd really appreciate feedback on what I've got here. I will continue to work on flipping groups, but it might take a bit longer for me to get to. (I probably won't be able to work on it until after Christmas.) In the meantime, would we be able to merge these changes to allow single element flipping once I handle any feedback?

@rileyschnee rileyschnee marked this pull request as ready for review December 13, 2020 18:19
@rileyschnee rileyschnee marked this pull request as draft December 13, 2020 18:26
@lipis
Copy link
Member

lipis commented Dec 13, 2020

you should sync with master to resolve the conflicts.. do you know how?

@rileyschnee
Copy link
Contributor Author

Yep! I think I got all of them here.

@rileyschnee
Copy link
Contributor Author

Alright...so now that I've rebased, there are issues because flipping does not have a key shortcut. Is this something we should add or is there a workaround? Currently, the number of expected context menu items is based on a list of shortcuts given.
Screen Shot 2020-12-13 at 2 03 42 PM

@rileyschnee
Copy link
Contributor Author

For now, I just added 2 to the expected context menu items. If we decide to add a keybinding, it can be added later.

I'm unfamiliar with the changelog check. Any advice for resolving that?

@rileyschnee rileyschnee marked this pull request as ready for review December 13, 2020 19:15
@ad1992
Copy link
Member

ad1992 commented Dec 13, 2020

For now, I just added 2 to the expected context menu items. If we decide to add a keybinding, it can be added later.

I'm unfamiliar with the changelog check. Any advice for resolving that?

@rileyschnee since we have a package @excalidraw/excalidraw and this change is part of the package so the changelog needs to be updated.

@lipis
Copy link
Member

lipis commented Dec 13, 2020

It shouldn't move the element.. right?

Kapture 2020-12-13 at 22 04 39

@lipis
Copy link
Member

lipis commented Dec 13, 2020

As for the shortcuts (Figma):

Screenshot 2020-12-13 at 10 07 07 PM

@lipis lipis changed the title Add option to flip single element on the context menu feat: Add option to flip single element on the context menu Dec 13, 2020
@dwelle
Copy link
Member

dwelle commented Feb 24, 2021

how I can access that here in the code? Or will I need to account for this some other way?

I haven't worked with lines for a while. If getting the bounding box is all you need, using getElementBounds() helper should be enough. If you need something custom, you can check how the getLinearElementRotatedBounds() is implemented (which is what getElementBounds() is using).

@rileyschnee
Copy link
Contributor Author

rileyschnee commented Feb 26, 2021

Fixed the issue with two-point arrow/lines shifting with flipping. However, because the x/y is based on the origin point for drawings and 3+ point lines where the origin isn't always in a corner, achieving the in-place flip is harder. Would appreciate feedback on current version.
Something that I need to do before this is merged is either get groups of elements to flip or disable the hotkeys from flipping groups of elements. I'm leaning in favor of the second option and then if @narendrasinghrathore is interested in working on flipping groups, then I'd be happy to give info on what that'll require once this PR is merged.

@narendrasinghrathore
Copy link

narendrasinghrathore commented Feb 27, 2021

@rileyschnee, I am interested to work on flipping groups of elements.
Let's have a discussion after this PR get merged.

@dwelle
Copy link
Member

dwelle commented Feb 27, 2021

@rileyschnee ok, disregarding the flip-in-place for multi-point lines, I noticed these issues:

  1. horizontally flipping 2-point line/arrow doesn't work:

    excal_flip_bug_2point_horizontal

  2. vertically flipping multi-point line makes it look like horizontal flip, even though it is flipped vertically. I guess this falls under the flip-in-place issue, though.

    excal_flip_bug_3point_vertical

@dwelle
Copy link
Member

dwelle commented Feb 27, 2021

Would appreciate feedback on current version.

I'll try to look at it later, though time is something I'm in dire scarcity of these days. Btw, the part of the codebase dealing with points positions & rotations is hard. I believe there's like one person on the team that's remotely competent in there (and it's not me).

@rileyschnee
Copy link
Contributor Author

@dwelle Thanks for taking a look! That arrow flipping issue kinda came out of nowhere. I'll fix that, but no rush on getting this accepted. @narendrasinghrathore and I can talk about group flipping in the meantime.

@narendrasinghrathore
Copy link

@narendrasinghrathore and I can talk about group flipping in the meantime.

@rileyschnee please let me know how would you like to setup a meeting. My email address nsrathore0212@gmail.com.
Thanks

@rileyschnee
Copy link
Contributor Author

@lipis I made the changes, so this is ready to review. The issues you pointed out should be resolved on the most recent vercel deployment, but let me know if you come across any issues. I don't have the ability to reopen this PR for review and pressing the suggested changes button from when you requested changes before now don't lead anywhere.

@andyworsley
Copy link

@rileyschnee Thanks for working on this! I've just discovered Excalidraw, and immediately went looking for this function.

@dwelle
Copy link
Member

dwelle commented Mar 21, 2021

Thanks for the reminder that I should review & look at the rotation transforms. Hopefully in the first half of next week. Sorry again for the delay!

Copy link
Member

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

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

looks great @rileyschnee 🎉

@dwelle
Copy link
Member

dwelle commented Mar 26, 2021

Let's ship! Thanks @rileyschnee for bearing with us. I'm sure your next contributions will have a much swifter response from us.

@dwelle dwelle merged commit b0d7ff2 into excalidraw:master Mar 26, 2021
@rileyschnee
Copy link
Contributor Author

Amazing! Thank you all for your support on this feature! I learned a lot!

@oe8bck
Copy link

oe8bck commented Mar 30, 2021

Thanks a lot for this feature I! I need it a lot!

But why isn't it working for a group of items? So if I include something from the library, I have to flip every single line, or do I miss something?

@ad1992
Copy link
Member

ad1992 commented Mar 30, 2021

But why isn't it working for a group of items? So if I include something from the library, I have to flip every single line, or do I miss something?

Flipping is currently supported only on single elements. Support for multiple elements will be added soon.

@baptiste
Copy link

baptiste commented Jan 4, 2022

For text elements, I think it might be preferable to mirror-image the drawing rather than rotate it by 180 degrees (one could always rotate to get the current effect, but there's no way to get mirrored text). If it's tricky as a text element (I have no idea) maybe a transformation to SVG paths can be the way to go? (as in #2993)

@dwelle
Copy link
Member

dwelle commented Jan 4, 2022

@baptiste if we do support text-mirroring we'd should make this behavior happen only when a single text element is selected. When we support flipping multiple elements, and you flip this:

image

I think you'd wanna arrive at:

image

rather than:

image

Figma mirrors the text in all cases, but I'd happily go against the flow in this case.

@baptiste
Copy link

baptiste commented Jan 5, 2022

@dwelle seems like a reasonable strategy (I guess it would depend a lot on the type of drawing – for diagrams, this does seem best); for single-text elements I think true mirroring would be useful

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.

Feature Request: Flip button for arrows/any shapes/a group of them
9 participants