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

Make copy paste data agnostic #909

Merged
merged 4 commits into from
Feb 1, 2023
Merged

Make copy paste data agnostic #909

merged 4 commits into from
Feb 1, 2023

Conversation

TrySound
Copy link
Member

Copy paste of instances was implemented closly coupled to our schema. Here I make I implement it with simpler interface without dependency on types. Type and version are instead passed as configuration.

Next step will be decoupling instance copy paste from react and simplifying data management with nanostores.

Code Review

  • hi @kof, I need you to do
    • conceptual review (architecture, feature-correctness)
    • test it on preview
  • hi @rpominov, I need you to do
    • detailed review (read every line)

Before requesting a review

  • made a self-review
  • added inline comments where things may be not obvious (the "why", not "what")

Before merging

  • tested locally and on preview environment (preview dev login: 5de6)
  • updated test cases document
  • added tests
  • if any new env variables are added, added them to .env.example and the designer/env-check.js if mandatory

@TrySound TrySound requested review from kof and rpominov January 31, 2023 19:38
@vercel
Copy link

vercel bot commented Jan 31, 2023

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

Name Status Preview Comments Updated
webstudio-designer ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Jan 31, 2023 at 10:05PM (UTC)

@kof
Copy link
Member

kof commented Jan 31, 2023

Copy paste of instances was implemented closly coupled to our schema. Here I make I implement it with simpler interface without dependency on types. Type and version are instead passed as configuration.

I miss the information "why" we need it, what does it unblock?

@kof
Copy link
Member

kof commented Jan 31, 2023

Do I understand correctly that we are executing copy/paste logic on both, designer and canvas? and you are trying to make it react decoupled?

We could do this much easer by simply forwarding the copy/paste event and having the logic in designer only. Basically always trying to just forward events from canvas and that's it.

Copy paste of instances was implemented closly coupled to our schema.
Here I make I implement it with simpler interface without dependency on
types. Type and version are instead passed as configuration.

Next step will be decoupling instance copy paste from react and
simplifying data management with nanostores.
@TrySound
Copy link
Member Author

The main idea is to split all copy/paste setup from data which was spread over few modules.
This will let me easier migrate to style sources.

Copy link
Contributor

@rpominov rpominov left a comment

Choose a reason for hiding this comment

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

Couple comments with not strong opinions. Up to you what to do with them :)

@TrySound TrySound merged commit abee2f1 into main Feb 1, 2023
@TrySound TrySound deleted the raw-copy-paste branch February 1, 2023 09:45
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