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

Create block interactive template (discarded) #52549

Conversation

juanmaguitar
Copy link
Contributor

What?

Creation of the "create-block-interactive-template" and addition of "view.js" to the default "create-block"

Why?

For the reasons explained here

Copy link
Contributor

@ryanwelcher ryanwelcher left a comment

Choose a reason for hiding this comment

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

The template code looks great and I love the addition of viewScript. Before we merge, not sure if we need another review in regards to this adding a new package.

@ryanwelcher
Copy link
Contributor

cc @gziolo

@ryanwelcher
Copy link
Contributor

ryanwelcher commented Jul 12, 2023

@juanmaguitar it looks like we need to update the Create Block test to account for the new view.js file being added.

Copy link
Member

@luisherranz luisherranz left a comment

Choose a reason for hiding this comment

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

Hey, Juanma. The template code looks good to me but I don't understand why this PR is built on top of a branch that contains part of the variant PR instead of trunk. It's not clear to me what's going to be merged and why it will end up in a new branch. From the variant PR, we only need the viewScript property propagation.

Would it be worth switching the base of this PR to trunk so we can clearly see what's included? Thanks!!

@@ -0,0 +1,48 @@
{{#isBasicVariant}}
Copy link
Member

Choose a reason for hiding this comment

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

I see double-line breaks in this file. Is it on purpose?


toggle: ( { context } ) => {

context[ '{{namespace}}' ].isOpen = !context[ '{{namespace}}' ].isOpen;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
context[ '{{namespace}}' ].isOpen = !context[ '{{namespace}}' ].isOpen;
context[ '{{namespace}}' ].isOpen = ! context[ '{{namespace}}' ].isOpen;

There's a missing space after the ! (my fault).

@juanmaguitar
Copy link
Contributor Author

I'm closing this PR in favor of this other PR #52612

@juanmaguitar
Copy link
Contributor Author

juanmaguitar commented Jul 13, 2023

Hey, Juanma. The template code looks good to me but I don't understand why this PR is built on top of a branch that contains part of the variant PR instead of trunk. It's not clear to me what's going to be merged and why it will end up in a new branch. From the variant PR, we only need the viewScript property propagation.

Would it be worth switching the base of this PR to trunk so we can clearly see what's included? Thanks!!

@luisherranz You're right. That PR was an attempt of using the same branch and the same PR fo these changes.
But it's definitely more clear and easy to create a new branch and a new PR on top of trunk like #52612

@luisherranz
Copy link
Member

Thank you, Juanma. I've reviewed the other PR 🙂

@juanmaguitar juanmaguitar changed the title Create block interactive template Create block interactive template (discarded) Jul 14, 2023
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