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: composite ordering #679

Merged
merged 4 commits into from
Jul 14, 2023
Merged

feat: composite ordering #679

merged 4 commits into from
Jul 14, 2023

Conversation

nicoecheza
Copy link
Contributor

@nicoecheza nicoecheza commented Jul 10, 2023

closes decentraland/sdk#871

πŸ€– Generated by Copilot at 18a5aec

Summary

πŸŒ²πŸ› οΈπŸšš

This pull request adds drag and drop functionality to the Hierarchy and ProjectView components, and introduces a new Nodes component to store and manipulate the entity hierarchy in the scene. It also updates the useTree hook, the @dcl/inspector operations, and some migration functions to use the Nodes component. It fixes some bugs and code style issues, and adds some tests and utils for the new features.

We're haulin' on the Nodes component, boys and girls
We're draggin' and droppin' the entities in the world
We're fixin' up the Tree and the Hierarchy
We're workin' on the editor, one, two, three

Walkthrough

  • Add Nodes component to store the hierarchy of the entities in the scene (link, link, link, link, link)
  • Add Node type to represent a node in the hierarchy, with an entity and a children property (link, link)
  • Add DropType type and calculateDropType function to handle different types of drop events for the drag and drop feature of the Tree component (link, link)
  • Add DisclosureWidget component to render the arrow icon that indicates if a node can be expanded or collapsed (link)
  • Add canDrag and canReorder props to the Tree component, to determine if a node can be dragged and reordered (link, link, link, link, link, link)
  • Add canDrag and canReorder functions to the useTree hook, to provide some validation logic for the drag and drop feature (link)
  • Add reorder function to the operations object, to reorder the nodes in the Nodes component value based on the source, target, and new parent entities (link, link)
  • Add runMigrations function to perform some migrations on the engine context, such as removing the old EntityNode components and building the Nodes component value if it does not exist (link, link)
  • Add buildNodesHierarchy and buildNodesHierarchyIfNotExists functions to build the Nodes component value from the current engine status (link)
  • Add pushChild and removeNode functions to add or remove a node from the tree, and return the updated tree (link, link)
  • Add tests for the buildNodesHierarchy and buildNodesHierarchyIfNotExists functions, using different composites and engine contexts (link)
  • Rename onSetParent prop to onDrop in the Hierarchy and ProjectView components, to reflect the new logic of handling different types of drop events (link, link)
  • Modify the useDrop hook logic in the Tree component, to use the calculateDropType function and the canReorder function, and to pass the drop type value to the onDrop prop function (link)
  • Modify the setParent function in the useTree hook, to use the getNewParent function and the reorder function, and to handle different types of drop events (link)
  • Modify the useTree hook logic, to use the Nodes component instead of the Transform component, and to return an array of nodes (link, link, link)
  • Modify the addChild function, to return the child entity and to update the Nodes component value with the pushChild function (link)
  • Modify the removeEntity function, to update the Nodes component value with the removeNode function for every entity iterator in the component entity tree (link)
  • Modify the generateMinimalComposite and generateMainComposite functions, to add the Nodes component to the engine and set its value to the minimal or main hierarchy (link, link)
  • Modify the createTempEngineContext and generateMinimalComposite functions, to export them as named exports (link, link)
  • Modify the generateMainComposite function, to add the gltfEntity to the engine before creating its components, and to remove the duplicate line that adds it again (link, link)
  • Modify the Tree component and its CSS styles, to add some border styles for different drop types and to adjust the margin and padding for the tree and the item area (link, link, link, link)
  • Modify the Tree component, to use a ref for the item area element and to pass the drag and drop functions to it (link, link, link)
  • Modify the Tree component, to use a custom component for the disclosure widget and to simplify the rendering of the children nodes (link, link)
  • Modify the initRpcMethods function, to remove the call to the removeLegacyEntityNodeComponents function (link)
  • Modify the removeLegacyEntityNodeComponents function, to move it to the migrations folder (link)
  • Remove the trailing comma from the jest-puppeteer.config.js file (link)
  • Remove the import statement for the removeLegacyEntityNodeComponents function from the rpc-methods.ts file (link)

@nicoecheza nicoecheza changed the title Feat/composite ordering feat: composite ordering Jul 10, 2023
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 10, 2023

Deploying with Β Cloudflare Pages Β Cloudflare Pages

Latest commit: f05885d
Status:Β βœ…Β  Deploy successful!
Preview URL: https://dfa379e2.js-sdk-toolchain.pages.dev
Branch Preview URL: https://feat-composite-ordering.js-sdk-toolchain.pages.dev

View logs

@nicoecheza nicoecheza force-pushed the feat/composite-ordering branch from e8965bf to ec95beb Compare July 10, 2023 17:06
@github-actions
Copy link
Contributor

github-actions bot commented Jul 10, 2023

Test this pull request

  • The @dcl/sdk package can be tested in scenes by running

    npm install "https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/composite-ordering/dcl-sdk-7.3.2-5554542224.commit-882f741.tgz"
  • To test with npx init

    export SDK_COMMANDS="https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/composite-ordering/dcl-sdk-commands-7.3.2-5554542224.commit-882f741.tgz"
    npx $SDK_COMMANDS init
  • The @dcl/inspector package can be tested by visiting this url

    • Or by installing it via NPM
    npm install "https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/composite-ordering/@dcl/inspector/dcl-inspector-7.3.2-5554542224.commit-882f741.tgz"
  • The /changerealm command to test test in-world

    /changerealm https://sdk-team-cdn.decentraland.org/ipfs/feat/composite-ordering-e2e
    
  • You can preview this build entering:
    https://playground.decentraland.org/?sdk-branch=feat/composite-ordering

@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Patch coverage: 76.39% and project coverage change: +0.39 πŸŽ‰

Comparison is base (62ae008) 74.12% compared to head (18cff3e) 74.51%.

❗ Current head 18cff3e differs from pull request most recent head f05885d. Consider uploading reports for the commit f05885d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #679      +/-   ##
==========================================
+ Coverage   74.12%   74.51%   +0.39%     
==========================================
  Files         306      311       +5     
  Lines       10256    10439     +183     
  Branches     1327     1352      +25     
==========================================
+ Hits         7602     7779     +177     
- Misses       2530     2535       +5     
- Partials      124      125       +1     
Impacted Files Coverage Ξ”
...ckages/@dcl/inspector/src/components/Tree/utils.ts 0.00% <0.00%> (ΓΈ)
packages/@dcl/inspector/src/hooks/sdk/useTree.ts 0.00% <0.00%> (ΓΈ)
...l/inspector/src/lib/data-layer/host/rpc-methods.ts 68.06% <ΓΈ> (-1.04%) ⬇️
...-layer/host/utils/migrations/legacy-entity-node.ts 100.00% <ΓΈ> (ΓΈ)
...s/@dcl/inspector/src/lib/sdk/operations/reorder.ts 47.61% <47.61%> (ΓΈ)
packages/@dcl/inspector/src/lib/sdk/tree.ts 85.71% <75.00%> (-6.53%) ⬇️
packages/@dcl/inspector/src/lib/utils/array.ts 84.21% <84.21%> (ΓΈ)
...yer/host/utils/migrations/build-nodes-hierarchy.ts 96.49% <96.49%> (ΓΈ)
...ector/src/lib/data-layer/client/feeded-local-fs.ts 100.00% <100.00%> (+17.96%) ⬆️
...r/src/lib/data-layer/host/utils/composite-dirty.ts 91.60% <100.00%> (+0.84%) ⬆️
... and 7 more

β˜” View full report in Codecov by Sentry.
πŸ“’ Do you have feedback about the report comment? Let us know in this issue.

@nicoecheza nicoecheza force-pushed the feat/composite-ordering branch 6 times, most recently from ec38548 to eeb7164 Compare July 12, 2023 19:13
@nicoecheza nicoecheza marked this pull request as ready for review July 12, 2023 19:14
@nicoecheza nicoecheza force-pushed the feat/composite-ordering branch 2 times, most recently from 1622038 to 26613a9 Compare July 12, 2023 19:58
@cazala
Copy link
Member

cazala commented Jul 12, 2023

I think there's a styling issue, when I drag a child over a parent it shows the indicator that it will be placed after the parent, but it moves it inside (where it already was, so it does nothing, but the indicator suggests that it would be moved outside, below the parent).

reorder.mp4

@nicoecheza
Copy link
Contributor Author

I think there's a styling issue, when I drag a child over a parent it shows the indicator that it will be placed after the parent, but it moves it inside (where it already was, so it does nothing, but the indicator suggests that it would be moved outside, below the parent).

reorder.mp4

yeah, it's the correct behaviour but the wrong UI. Will take a look at it πŸ‘

@nicoecheza nicoecheza force-pushed the feat/composite-ordering branch from ca09b28 to 18a5aec Compare July 13, 2023 14:24
@cazala
Copy link
Member

cazala commented Jul 13, 2023

I think when a node has children it's not possible to drag an entity after it? Look at this example, test1 has children (test2), and I can't drag test4 between test1 and test3:

reorder2.mp4

Also there seems to be 2 hover states? One that puts the dropped entity inside at the top and the other puts the entity inside at the bottom of the list.

I think it would also be nice to highlight somehow when an entity is going to be dropped inside another, because currently we have the "after" hightlight but the "inside" doesnt actually highlight anything

@nicoecheza nicoecheza force-pushed the feat/composite-ordering branch from 18a5aec to 18cff3e Compare July 14, 2023 13:14
@nicoecheza nicoecheza force-pushed the feat/composite-ordering branch from 18cff3e to f05885d Compare July 14, 2023 13:18
@nicoecheza
Copy link
Contributor Author

fixed the issue for dropping after parents + added a background color for dropping inside a parent that makes it more clear πŸ‘Œ

Screen Shot 2023-07-14 at 10 15 44

@nicoecheza nicoecheza merged commit ce2cb26 into main Jul 14, 2023
@nicoecheza nicoecheza deleted the feat/composite-ordering branch July 14, 2023 17:27
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.

Composite inspector: allow user to re-order entities by drag & dropping.
2 participants