Editor: Allow parent page to be removed. #896

timmyc · 2015-11-27T15:51:35Z

Fixes #741

Currently in the editor there is no way to remove a page parent if one is currently set. This branch adds a checkbox above the Parent Page Selector, much like we show in the Add Category screen, to allow removing the parent page option, or setting it back to a "Top level page.".

To Test

Open up a new page in the editor
Expand the Page Options accordion, note that "Top level page" is checked
Add a title and some content
Select a page parent, note the checkbox is un-checked
Save the page, and do a hard refresh, check that your parent page selection is persisted
Check the Top Level Page checkbox, note that the parent page radio is de-selected
Save the page, hard refresh and note that the page is still set to Top Level

mtias · 2015-11-27T15:53:23Z

client/post-editor/editor-page-parent/index.jsx

-				</label>
+				</FormLabel>
+				<FormLabel>
+					<FormCheckbox ref="topLevel" checked={ ! this.props.parent } onChange={ this.updatePageParent } />


We should use a toggle for these singular controls.

folletto · 2015-11-27T15:57:26Z

Would be possible to add that as an option inside the box, at the top, with a house icon? The reason is that in terms of structure you're just positioning it to the very top, so it's a "top" option in the parent page structure (root).

Not sure if it's better to indent or not there. Opinions?

timmyc · 2015-11-27T16:11:09Z

In 002239f use of the toggle, and styling that is currently seen in "Post Status" was added:

@folletto I believe we might have originally gone that route with the Add Category screen too, and I seem to think we did not go that way due to the nature of checkboxes/multi-select. Here though, that approach could work with the radios.

I kind of like this toggle approach myself.

folletto · 2015-11-27T16:16:59Z

The item inside the list has two big advantages:

It's semantically correct, being the root element
It's visually cleaner

Unless there's a big technical issue, we should really try to get it inside the list. :)

timmyc · 2015-11-27T16:25:24Z

@mtias do you have an opinion ^ I can create a prop for an empty/null option in <PostSelector /> to accomplish this. @folletto having that as an option inside the selector, would it be hidden or shown when search results are displayed?

mtias · 2015-11-27T16:37:45Z

@timmyc I'd get the top level toggle in so we have something and then look at including it within the list, since it won't be as trivial.

timmyc · 2015-11-27T16:45:15Z

Sounds like a good plan @mtias

folletto · 2015-11-27T16:45:30Z

Agreed 👍

jkudish · 2015-12-08T16:50:59Z

Quick notes from a quick test sesh:

it's kinda weird that it's a toggle but i can't untoggle it
I selected a page parent, then changed my mind and set it back to top level and clicked published, but it published with that initial selection of a parent page
After I published the page, I had the option of setting the page parent to itself, which is weird. When I selected it, it reverted to top level and that option went away.
Not sure if related or not, but after making a few rounds of updates I got this (no console errors):

timmyc · 2015-12-08T17:39:50Z

@jkudish I can't seem to re-create the issue above where you select a page, then click the top level toggle and the old selection is persisted. Additionally I'm not seeing the newly created page being shown as an option in the selector.

I suppose there might be a brief moment when the post-edit-store does not yet have an ID for the post - and possibly a new post-list-store payload comes down with the new page inside of it - but I really can't think of how that might happen.

As for the draft save failure - I added an extra check to updatePageParent() in e0f5536 to make sure the argument being passed in is a page with an ID. The method was taking the event argument and passing undefined to the API which was making things 💥

jkudish · 2015-12-09T00:46:44Z

I just re-tested. Looking better. I can no longer repro 2/3 of the bugs from above.

I can still repro this one:

After I published the page, I had the option of setting the page parent to itself, which is weird. When I selected it, it reverted to top level and that option went away.

This time I got it by editing a published page which had a parent page and reverting it to a draft.

One other bug I just noticed, though maybe it's for a separate PR: the previewed permalink of a page with a parent should be updated to reflect its page parent. If I have a Page B with Page A as its parent, the permalink is http://example.wordpress.com/page-a/page-b but currently the editor shows the permalink as http://example.wordpress.com/page-b.

timmyc · 2015-12-09T20:41:38Z

The slug thing is a definite regression - that was working at one point during the great slug explorations of 2015. I will spawn an issue for that.

As for the page being shown as an option in the parent list - I finally was able to reproduce this. I had to let the editor site idle for a few minutes for this to happen though. But it appears another component polls the post-list-store:

https://public-api.wordpress.com/rest/v1.1/sites/91836948/posts?http_envelope=1&status=publish,private&type=page&site_visibility=visible&modified_after=2015-12-09T20:31:33+00:00

The key with this request is it does not include exclude_tree with a value of the page.ID. The api then returns the page in this request since it matches the query and is not excluded. This particular bug is kind of out of scope of this PR - but I am guessing it is either the scheduler which seems to be enabled now on pages .

Though, this bug would be fixed by #708 since each component would have their own post-list-store to work against then.

timmyc · 2015-12-11T15:24:02Z

@jkudish mind taking one last look at this - well no additional commits but I created a new issue to track the parent slug bit - and explained the other issue you were seeing above.

aduth · 2015-12-11T18:21:26Z

client/post-editor/editor-page-parent/index.jsx

 	},

 	render() {
 		return (
 			<AccordionSection className="editor-page-parent">
-				<label>
+				<FormLabel>
 					<span className="editor-page-parent__label-text">{ this.translate( 'Parent Page' ) }</span>


Unrelated to this pull request, but seems like we could move the className to the <FormLabel> component and have it work just as well.

aduth · 2015-12-11T18:34:20Z

Though, this bug would be fixed by #708 since each component would have their own post-list-store to work against then.

Hmm, I'm still seeing this bug even after having rebased against latest master, with #708 having been merged.

timmyc · 2015-12-11T19:05:21Z

Hmm, I'm still seeing this bug even after having rebased against latest master, with #708 having been merged.

That is because #708 kept all usage of post-list-store in the default sense ( so shared ). I will add a change to this PR to scope it to the post-selector.

timmyc · 2015-12-11T19:12:23Z

@aduth utilizing the new post-list-store factory abilities in 29213e6 and removed unused className in 7ed3a2b

jkudish · 2015-12-12T01:32:23Z

On a site with only a few pages, I don't the box around the pages, I am guessing this is expected:

Other than that (which I don't think is a bug), and the noted issues above, I think this is good to go.

mtias · 2015-12-12T13:38:21Z

On a site with only a few pages, I don't the box around the pages, I am guessing this is expected:

Yes, few pages don't require the box nor search. Same with few categories.

Editor: Allow parent page to be removed.

timmyc self-assigned this Nov 27, 2015

mtias reviewed Nov 27, 2015
View reviewed changes

timmyc mentioned this pull request Nov 27, 2015

PostSelector: Create Empty/Null Prop #902

Closed

jkudish added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 8, 2015

timmyc added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Dec 8, 2015

timmyc mentioned this pull request Dec 9, 2015

Editor: Page Slugs Do Not Reflect Parents #1439

Closed

aduth reviewed Dec 11, 2015
View reviewed changes

timmyc added 3 commits December 11, 2015 11:00

Editor: Allow parent page to be removed.

4e83ae1

Use toggle for page parent.

68a5635

Ensure there is an item && item.ID when changing page parent.

4c94b5d

Utlizie specific post-list-store for post-selector.

29213e6

timmyc force-pushed the update/editor/page-parent branch from e0f5536 to 29213e6 Compare December 11, 2015 19:09

Remove unused gridicon className.

7ed3a2b

jkudish added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 12, 2015

timmyc added a commit that referenced this pull request Dec 14, 2015

Merge pull request #896 from Automattic/update/editor/page-parent

b5c95ea

Editor: Allow parent page to be removed.

timmyc merged commit b5c95ea into master Dec 14, 2015

scruffian removed the [Status] Ready to Merge label Dec 14, 2015

timmyc deleted the update/editor/page-parent branch December 14, 2015 17:48

lancewillett removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Oct 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Allow parent page to be removed. #896

Editor: Allow parent page to be removed. #896

timmyc commented Nov 27, 2015

mtias Nov 27, 2015

timmyc Nov 27, 2015

folletto commented Nov 27, 2015

timmyc commented Nov 27, 2015

folletto commented Nov 27, 2015

timmyc commented Nov 27, 2015

mtias commented Nov 27, 2015

timmyc commented Nov 27, 2015

folletto commented Nov 27, 2015

jkudish commented Dec 8, 2015

timmyc commented Dec 8, 2015

jkudish commented Dec 9, 2015

timmyc commented Dec 9, 2015

timmyc commented Dec 11, 2015

aduth Dec 11, 2015

aduth commented Dec 11, 2015

timmyc commented Dec 11, 2015

timmyc commented Dec 11, 2015

jkudish commented Dec 12, 2015

mtias commented Dec 12, 2015

Editor: Allow parent page to be removed. #896

Editor: Allow parent page to be removed. #896

Conversation

timmyc commented Nov 27, 2015

mtias Nov 27, 2015

Choose a reason for hiding this comment

timmyc Nov 27, 2015

Choose a reason for hiding this comment

folletto commented Nov 27, 2015

timmyc commented Nov 27, 2015

folletto commented Nov 27, 2015

timmyc commented Nov 27, 2015

mtias commented Nov 27, 2015

timmyc commented Nov 27, 2015

folletto commented Nov 27, 2015

jkudish commented Dec 8, 2015

timmyc commented Dec 8, 2015

jkudish commented Dec 9, 2015

timmyc commented Dec 9, 2015

timmyc commented Dec 11, 2015

aduth Dec 11, 2015

Choose a reason for hiding this comment

aduth commented Dec 11, 2015

timmyc commented Dec 11, 2015

timmyc commented Dec 11, 2015

jkudish commented Dec 12, 2015

mtias commented Dec 12, 2015