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

Add template part previews to placeholder block #22760

Merged
merged 53 commits into from
Jun 16, 2020

Conversation

Addison-Stavlo
Copy link
Contributor

@Addison-Stavlo Addison-Stavlo commented May 29, 2020

Description

An alternative exploration to #22612 as a potential step forward for issue #21218. Here, I have added the previews and selectable interface to the Template Part placeholder block. A handful of styles and use-async-list file were carried over from the inserter implementation.

inserting-tps

The placeholder now has 2 sections divided by Tabs: one for selecting via previews and the other for creating the new template part.

The template part creation flow has been 'mostly' untouched for the time being. For this section I have removed the preview, updated the choose/create button and function to only support 'create', and updated the help text to guide the user.

creation-tps

This seems like a good step forward, but this will probably require some future iteration in style and creation flow in upcoming PRs.

How has this been tested?

Local docker environment in site/post editors.

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@epiqueras
Copy link
Contributor

Let's follow this up with: #22760 (comment).

@Addison-Stavlo Addison-Stavlo merged commit c0ea950 into master Jun 16, 2020
@Addison-Stavlo Addison-Stavlo deleted the add/template-part-previews-to-placeholder branch June 16, 2020 18:59
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 16, 2020
@diegohaz
Copy link
Member

diegohaz commented Jun 16, 2020

I'm seeing this error when running npm run build:

Error: Undefined variable: "$theme-color".
    at options.error (/Users/haz/Projects/gutenberg/node_modules/node-sass/lib/index.js:291:26) {
  type: 'Error',
  '$error': '$error',
  status: 1,
  file: '/Users/haz/Projects/gutenberg/packages/block-library/src/template-part/editor.scss',
  line: 67,
  column: 32,
  formatted: 'Error: Undefined variable: "$theme-color".\n' +
    '        on line 67 of packages/block-library/src/template-part/editor.scss\n' +
    '        from line 46 of packages/block-library/src/editor.scss\n' +
    '>> \t\t\tborder: $border-width solid $theme-color;\n' +
    '\n' +
    '   -------------------------------^\n'
}

In fact, it seems like $theme-color is not defined. Maybe a rebase issue?

Update: #23217

@noahtallen
Copy link
Member

Maybe a rebase issue?

it must have been something in the merge since the build was passing here

@epiqueras
Copy link
Contributor

Super weird.

@Addison-Stavlo
Copy link
Contributor Author

I am seeing this in travis builds on another PR (#23007), but I started seeing it a couple hours before this PR merged? its definitely confusing me here...

@Addison-Stavlo
Copy link
Contributor Author

I cant build master because of this error. I wonder why $theme-color works in various places in the inserter:


but now having it here is failing.

@noahtallen
Copy link
Member

maybe a file needs imported?

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Jun 16, 2020

#23221 should fix it.

And that permalink I noted above was on the wrong branch, it looks like they changed to var(--wp-admin-theme-color) 11 hours ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants