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

Try @wordpress/edit-post for the creator UI. #115

Merged
merged 66 commits into from
Jun 9, 2021

Conversation

StevenDufresne
Copy link
Collaborator

@StevenDufresne StevenDufresne commented May 27, 2021

This PR is an experiment to test the use of import { initializeEditor } from '@wordpress/edit-post';

@ryelle Since I didn't author all the changes in the branch, can you take a really critical eye?

Closes #59.

Implements:

  • Various style updates

Doesn't yet Implement:

Screenshots

How to test the changes in this Pull Request:

  • Visit /new-pattern/
  • Add a title
  • Add some blocks
  • Add a description
  • Select some categories
  • Change the viewport width
  • Select block scope

In the browser console, paste the following one by one:

wp.data.select('core/editor').getEditedPostAttribute('title')
wp.data.select('core/editor').getEditedPostAttribute('pattern-categories')
wp.data.select('core/editor').getEditedPostAttribute('meta')

Expect to see your updates values.

ryelle and others added 27 commits November 25, 2020 18:05
# Conflicts:
#	package.json
#	public_html/wp-content/plugins/pattern-creator/pattern-creator.php
#	public_html/wp-content/plugins/pattern-creator/src/components/editor/index.js
#	public_html/wp-content/plugins/pattern-creator/src/components/header/index.js
#	public_html/wp-content/plugins/pattern-creator/src/components/header/style.css
#	public_html/wp-content/plugins/pattern-creator/src/components/layout/index.js
#	public_html/wp-content/plugins/pattern-creator/src/components/layout/style.css
#	public_html/wp-content/plugins/pattern-creator/src/components/settings/panels.js
#	public_html/wp-content/plugins/pattern-creator/src/index.js
#	public_html/wp-content/plugins/pattern-creator/src/store/actions.js
#	public_html/wp-content/plugins/pattern-creator/src/store/selectors.js
#	public_html/wp-content/plugins/pattern-creator/src/style.css
#	public_html/wp-content/plugins/pattern-creator/view/editor.php
#	yarn.lock
@StevenDufresne StevenDufresne requested a review from ryelle May 27, 2021 05:41
@StevenDufresne
Copy link
Collaborator Author

I see another issue that needs to be addressed before we can merge this approach. Clicking "Save Draft" updates the page URL to site.com/new-pattern/post.php?…, which is a page that doesn't exist. A better flow would be to update the URL to the correct /pattern/{whatever}/edit, or at least disable the change. This came from BrowserURL when I looked into this last year (#26).

Can this be sorted with a rewrite? I don't think there's a sane way to stop it from happening on the front apart from changing it back.

@iandunn
Copy link
Member

iandunn commented Jun 3, 2021

I haven't dug deep into it, so take this all w/ a grain of salt, but a rewrite sounds like a good workaround if the JS isn't configurable/overwritable.

Some alternative ideas if any of them seem useful:

  • submit a PR to Gutenberg to make the component more customizable. maybe adding a JS filter to getPostEditURL()
  • add a JS event handler to catch the location change, prevent the default action, and go directly to the correct URL. or one to run after the location change, and update the URL to the correct one
  • replace the Save Draft button w/ a custom one, if that's possible?
  • Could maybe also use a PHP filter the edit-post URL on before it's passed to JS

I'd rely on Kelly's thoughts more than any of that, though.

@StevenDufresne
Copy link
Collaborator Author

Much appreciated!

Some alternative ideas if any of them seem useful:

  • submit a PR to Gutenberg to make the component more customizable. maybe adding a JS filter to getPostEditURL()

This seems like a good idea. I wonder how long it would take to do this (or is it possible?). @ryelle Any opinion on this?

  • add a JS event handler to catch the location change, prevent the default action, and go directly to the correct URL. or one to run after the location change, and update the URL to the correct one

Can't do much here since we call replaceState directly in JS (we can't listen for popstate) and do nothing in Gutenberg's state object. We could maybe replace window.history.replaceState but it's really risky and we'll most likely get the intercept wrong in some contexts.

  • replace the Save Draft button w/ a custom one, if that's possible?

We can replace it but the control that updates the URLs listens for a change to draft and then triggers so we wouldn't be able to change the post status.

  • Could maybe also use a PHP filter the edit-post URL on before it's passed to JS

Not familiar with how this would work. @ryelle Any opinion on this?

@ryelle
Copy link
Contributor

ryelle commented Jun 4, 2021

Can this be sorted with a rewrite?

That would work, though I don't love the idea. We'll need to check for a post ID and load in the pattern content into the editor on new-pattern as well as when editing a pattern, but it's a good workaround.

Could maybe also use a PHP filter the edit-post URL on before it's passed to JS

Unfortunately it's not pulled from PHP, the redirect to post.php is hardcoded in getPostEditUrl.

submit a PR to Gutenberg

I wonder how long it would take to do this (or is it possible?)

It's definitely possible, but probably depends on whether we can prove our use-case to whoever reviews it. We have a real use-case here, so that's good. If you do create an issue, I suggest explaining the use case and making some suggestions, rather than jumping to a PR. We could shortcut out of BrowserUrl entirely, or add a JS filter, or figure out some way to get the URL from the post API, so that we can edit the url in PHP.

This feels like a pretty big blocker to this approach, so I'd like to have a plan before this is merged.

@StevenDufresne
Copy link
Collaborator Author

I was able to get it working using redirects. I don't think it's the best approach, I think getting something into Gutenberg would be a better approach long term but it does work and isn't a bad experience.

What do y'all think?

Here is the code:
https://github.com/WordPress/pattern-directory/compare/try/edit-post-package-first...try/edit-post-package-redirect?expand=1

Here is what it looks like functionally:
View GIF

@iandunn
Copy link
Member

iandunn commented Jun 7, 2021

🤔 , I wonder if the explicit RewriteRule in .htaccess and add_rewrite_rule() are necessary? Production uses nginx, so we'd need a sysreq to have similar functionality, but I'm guessing the wp_safe_redirect() would work w/out it.

if ( isset( $_GET['post'] ) && isset( $_GET['action'] ) && 'edit' === $_GET['action'] ) {

You might need to make sure that the post ID is a pattern post type, and that the user isn't in wp-admin, to avoid redirecting other post types and manually adding patterns.

It's odd that there's the FOUC, doing the wp_safe_redirect() in init should short-circuit that 🤔

@ryelle
Copy link
Contributor

ryelle commented Jun 7, 2021

I think that RewriteRule pops up any time you refresh permalinks, I've had to discard it before.

Do we need a sysreq for add_rewrite_rule? I thought WP itself handles those. Would the redirect to /pattern/[ID]/edit/ work without a rewrite rule to handle it?

It's odd that there's the FOUC, doing the wp_safe_redirect() in init should short-circuit that

I think that's the page before the JS content loads, not to do with the redirect, unless I'm misunderstanding FOUC (flash of unstyled content?).

I don't think it's the best approach, I think getting something into Gutenberg would be a better approach long term but it does work and isn't a bad experience.

Yeah, I agree.

@StevenDufresne
Copy link
Collaborator Author

🤔 , I wonder if the explicit RewriteRule in .htaccess and add_rewrite_rule() are necessary? Production uses nginx, so we'd need a sysreq to have similar functionality, but I'm guessing the wp_safe_redirect() would work w/out it.

Yeah, the .htaccess rule seems to be automatically added on permalinks save. I reset the file, resaved the permalinks and it came back so I left it in. I have no clue why it keeps getting re-added. I just assumed it's something .wp-env needs. ❓ ❓

if ( isset( $_GET['post'] ) && isset( $_GET['action'] ) && 'edit' === $_GET['action'] ) {

You might need to make sure that the post ID is a pattern post type, and that the user isn't in wp-admin, to avoid redirecting other post types and manually adding patterns.

Good call, I'll make it more specific.

It's odd that there's the FOUC, doing the wp_safe_redirect() in init should short-circuit that 🤔

Yeah, that flash is the page itself, it's technically the wp header on the page without styles. The react app appears on top once the JS loads. We'll need to add a better loading state for that but the redirect does work as expected.

@iandunn
Copy link
Member

iandunn commented Jun 7, 2021

Ah, I see. You're right, we'd only need a sysreq if we needed an nginx rewrite similar to the .htaccess one.

@StevenDufresne
Copy link
Collaborator Author

I've added the redirect code to this branch and I think I've closed out all the main issues identified.

@ryelle ryelle self-requested a review June 8, 2021 13:57
Copy link
Contributor

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

A few small notes, but once those are addressed I think this can be merged so we can iterate on it in trunk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Component] Pattern Creator Anything related to the pattern front end editor or preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update creator UI based on mockup
4 participants