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.

Sign up for GitHub

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

Jump to bottom

feat: Expose ActionManager.registerAction through ExcalidrawImperativeAPI #6995

Merged
merged 3 commits into from
Nov 22, 2023
Merged

feat: Expose ActionManager.registerAction through ExcalidrawImperativeAPI #6995

merged 3 commits into from
Nov 22, 2023

Conversation

DanielJGeiger
Copy link
Collaborator

For places where the "Rules of Hooks" preclude calling useExcalidrawActionManager().

@vercel
Copy link

vercel bot commented Sep 15, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Nov 4, 2023 0:45am
excalidraw ✅ Ready (Inspect) Visit Preview Nov 4, 2023 0:45am
excalidraw-package-example ✅ Ready (Inspect) Visit Preview Nov 4, 2023 0:45am

@github-actions
Copy link

github-actions bot commented Sep 15, 2023 •
edited
Loading

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
dist/excalidraw.production.min.js 313.92 KB (+0.01% 🔺) 6.3 s (+0.01% 🔺) 526 ms (+1.59% 🔺) 6.9 s
dist/excalidraw-assets/locales 280.73 KB (0%) 5.7 s (0%) 122 ms (+1.16% 🔺) 5.8 s
dist/excalidraw-assets/vendor*.js 826.02 KB (0%) 16.6 s (0%) 1.2 s (+11.43% 🔺) 17.8 s

@github-actions
Copy link

github-actions bot commented Sep 15, 2023 •
edited
Loading

Coverage Report

Status Category Percentage Covered / Total
🟢 Lines 73.41% / 70% 44336 / 60393
🟢 Statements 73.41% / 70% 44336 / 60393
🟢 Functions 69.65% / 68% 1421 / 2040
🟢 Branches 80.91% / 70% 5559 / 6870
File Coverage
File Stmts % Branch % Funcs % Lines Uncovered Lines
Changed Files
src/types.ts 100% 100% 100% 100%
src/components/App.tsx 73.16% 76.31% 71.68% 73.16% 416-417, 596-597, 613-614, 635-695, 698-704, 707-710, 713-785, 788-807, 815-827, 832-833, 837-839, 858-960, 1020-1021, 1058-1063, 1068-1113, 1118, 1136, 1145, 1152-1155, 1194-1195, 1264-1270, 1362-1367, 1370-1398, 1401-1441, 1487-1488, 1506-1507, 1544-1545, 1549-1550, 1566-1573, 1578-1591, 1597-1598, 1603-1611, 1613-1621, 1633, 1667, 1724-1725, 1729-1734, 1738-1764, 1769-1770, 1805-1808, 1886-1888, 1950-1951, 1964-1967, 1970, 1972-1973, 1980-1981, 1987-1988, 1996-1997, 2000-2001, 2004-2007, 2010-2013, 2024-2032, 2037-2038, 2094-2095, 2109-2115, 2121-2129, 2133-2141, 2145-2146, 2149-2186, 2189-2201, 2212-2213, 2224-2225, 2243-2247, 2251-2254, 2276-2283, 2286, 2288-2293, 2297-2299, 2315-2316, 2318-2327, 2353, 2359, 2398-2399, 2416-2418, 2440-2441, 2447-2450, 2456-2530, 2602-2603, 2608-2611, 2674, 2684-2702, 2705-2711, 2714-2715, 2721-2734, 2820-2821, 2823-2824, 2829-2831, 2856-2870, 2875-2894, 2917-2918, 2951-2953, 2978, 2997-3001, 3012-3013, 3016-3020, 3022-3023, 3025-3028, 3053-3054, 3063-3065, 3067, 3142-3145, 3165-3167, 3177-3178, 3180-3200, 3203-3209, 3226-3229, 3235-3238, 3248-3255, 3259-3260, 3265, 3284, 3289-3290, 3295-3299, 3331-3332, 3335-3336, 3344-3348, 3352-3362, 3367-3394, 3399-3410, 3469, 3495-3496, 3587, 3764-3765, 3768-3769, 3777, 3789, 3791-3792, 3833-3837, 3888-3894, 3900-3963, 4005, 4054, 4112, 4160-4163, 4166-4172, 4174-4177, 4192-4201, 4204-4205, 4286-4287, 4290, 4292-4293, 4299-4301, 4303, 4312, 4334-4339, 4341-4344, 4348-4349, 4359-4446, 4449-4450, 4464-4465, 4516-4517, 4544-4545, 4575-4601, 4608-4609, 4632-4633, 4653, 4655-4656, 4672-4673, 4679-4680, 4684-4687, 4690-4691, 4706-4729, 4737, 4739, 4741-4745, 4804-4809, 4811-4813, 4829, 4831-4842, 4867-4870, 4929-4930, 4932-4955, 4970-4971, 5083-5112, 5177-5181, 5204-5205, 5225-5226, 5319, 5325-5326, 5349-5368, 5383, 5516, 5539-5590, 5603, 5644-5650, 5665-5667, 5808-5812, 5836-5864, 5900, 5902-5909, 5916-5919, 5932, 5955-5956, 5959-5960, 5965-5967, 5970-5971, 5996-5997, 6025-6026, 6108-6109, 6158-6171, 6231-6234, 6260-6261, 6306, 6324-6330, 6337-6340, 6363-6368, 6439-6442, 6448, 6463-6470, 6473-6480, 6536, 6610-6611, 6631-6633, 6636, 6650-6674, 6780-6799, 6809-6848, 6861-6862, 6871-6878, 6892-6904, 6917-6922, 6974-7002, 7004-7005, 7079-7083, 7090, 7092-7128, 7157, 7207, 7226-7228, 7253-7258, 7260-7261, 7266-7297, 7300-7324, 7338-7339, 7345-7354, 7359, 7363-7367, 7376-7377, 7380-7385, 7389-7396, 7426, 7428-7432, 7434-7444, 7466-7474, 7478-7514, 7517-7587, 7595, 7613-7620, 7622-7638, 7653-7675, 7694-7696, 7741-7742, 7772-7773, 7789, 7863-7866, 7868-7884, 7886-7890, 7902-7903, 7913-7929, 7963-7964, 7968-7969, 7979, 7981-7984, 7986-7987, 8027, 8048-8049, 8075, 8078, 8118, 8129-8136, 8152-8153, 8185-8188, 8257-8267, 8269-8279, 8310-8317, 8343-8344, 8385-8439, 8483-8484, 8493-8496, 8501-8502, 8523-8525, 8527-8531, 8569
Generated in workflow #895

tomayac
tomayac approved these changes Oct 9, 2023
View reviewed changes
Copy link
Member

@tomayac tomayac left a comment

Choose a reason for hiding this comment

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

Non-owner LGTM

@dwelle should check this for potential side effects that I don't see.

@dwelle
Copy link
Member

dwelle commented Nov 3, 2023

My concern would be it has many APIs I'd not consider stable. Further, the manager itself is possibly gonna be completely rewritten with respect to upcoming changes we're planning.

Safer would be to expose just the executeAction method which is the main reason for exposing the manager @DanielJGeiger? Downside of doing this is if we later want to expose more actionManager methods (or actionManager as a whole) we'd either have to deprecate the executeAction or have some weird kludge of APIs we expose.

/cc @ad1992 thoughts?

@DanielJGeiger
Copy link
Collaborator Author

@dwelle You have a good point about not exposing unstable APIs. The main use is actually registerAction. I pushed an update here exposing only registerAction (see also c456c1e).

I use registerAction mainly while registering ExcalidrawElement subtypes in React hooks. But registerAction has broader interest: registering custom actions in general.

@dwelle
Copy link
Member

dwelle commented Nov 4, 2023

Mhm, I'd probably wait with merging this post 0.17.0 npm release which should happen towards the end of next week, as it's likely we'll be touching these APIs after that.

@dwelle dwelle added this to the @excalidraw/[email protected] milestone Nov 4, 2023
@DanielJGeiger DanielJGeiger changed the title feat: Expose ActionManager through ExcalidrawImperativeAPI feat: Expose ActionManager.registerAction through ExcalidrawImperativeAPI Nov 22, 2023
@DanielJGeiger DanielJGeiger merged commit d1e4421 into master Nov 22, 2023
10 checks passed
@DanielJGeiger DanielJGeiger deleted the api-expose-action-manager branch November 22, 2023 21:22
DanielJGeiger added a commit that referenced this pull request Nov 22, 2023
@DanielJGeiger
feat: Expose ActionManager.registerAction through `ExcalidrawImpera…
63a91a8 
…tiveAPI` (#6995)

* feat: Expose `ActionManager` through `ExcalidrawImperativeAPI`

* Only expose `registerAction` instead of `ActionManager`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomayac tomayac tomayac approved these changes

@dwelle dwelle Awaiting requested review from dwelle

@ad1992 ad1992 Awaiting requested review from ad1992

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
@excalidraw/[email protected]
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@DanielJGeiger @dwelle @tomayac