Editor: Update parent terms and pages selectors look and feel #23496

Copons · 2018-03-21T12:37:52Z

Make the parent term selector in the post editor and the parent page selector in the page editor as close as possible by replacing the top level checkbox with a toggle and by updating how its unchecked state is handled.

Parent Term Selector

Before	After

Notes

Apparently, terms labels are returned by the backend and stored in state; for this reason they are capitalized (e.g. "Choose a Parent Category").
I'm not exactly sure what is the best way to work around this, though.

To be fair, I think that changing how the backend creates those string is a bit overkill (from a dev standpoint), as it would require checking every single place they are used.

Probably it makes more sense to find labels that work better with capitalized words.

Parent Page Selector

Before	After

Notes

As you may notice by inspecting the changes, I had to pretty much rewrite this component.
The reason is that previously, the editPost action was dispatched every time the toggle changed and an item was selected from the tree, which was possible because you couldn't actually uncheck the top level toggle by clicking on the toggle, but only by selecting a parent page.

Now, it's possible to uncheck the top level toggle via the toggle itself, without selecting a parent page. Though, if we dispatched editPost now, we'd have no actual value for the parent page.

The new logic consists in the fact that unchecking the toggle doesn't dispatch editPost anymore, but simply make the parent page selector show up.
Only checking the toggle or selecting a parent page dispatches editPost.

Testing instructions

For parent terms:

Edit a post, toggle the settings sidebar, and expand the "Category & Tags" section.
Click on "Add New Category" to open the dialog.
Check if the top level toggle and parent selector feels good to use.

For parent pages:

Edit a page, toggle the settings sidebar, and expand the "Page Attributes" section.
Check if the top level toggle and parent selector feels good to use.

matticbot · 2018-03-21T12:37:53Z

Test live: https://calypso.live/?branch=fix/ui/8513-new-category-parent-page

a8ci18n · 2018-03-21T12:43:43Z

client/blocks/term-form-dialog/index.jsx

 					<span>
-						{ translate( 'Top level', { context: 'Terms: New term being created is top level' } ) }
+						{ translate( 'Top level %(term)s', {


ℹ️ This context is really long. Are you sure you don't want to use a translator comment instead?

Indeed, please convert context to a comment, thanks!

folletto · 2018-03-21T14:31:44Z

Aside: I'm ok with this change, but for the future: the setting on the Pages list specifically shouldn't be there imho.

Pages is a hierarchy with a root: the site front page / home page is the top level. As such, the "Top Level" shouldn't be a setting outside the list, but simply an option INSIDE the hierarchy in the list — that is: "being a children of the Home Page". It's very odd for the top level page to be a setting outside the page hierarchy. The current UI reflects instead the database structure.

Categories is instead different because category hierarchies don't have a "root".

/cc @drw158 @shaunandrews

kwight · 2018-03-21T16:24:55Z

This behaves a little oddly for me. I can set a new parent category, but then my current page ("Testy" in the screenshots) shows up as a new optional top page:

I can then click on it and update to make it my current page's top page to itself (in perception, at least):

What I would expect is to never see the current page, so that making it a top-level page or not can only be done by toggling "top level" on or off.

shaunandrews · 2018-03-22T19:41:08Z

As soon as I toggle the switch in the category modal, I get errors:

Also, the error at the bottom mentions the top level option being "unchecked" and the label on the switch is capitalizing "Category".

gwwar · 2018-04-13T19:53:24Z

@Copons what are next steps here?

Copons · 2018-04-18T16:11:57Z

what are next steps here?

@gwwar 😨 Why, rebase and address folks' concerns!

I can set a new parent category, but then my current page shows up as a new optional top page
I can then click on it and update to make it my current page's top page to itself

@kwight Addressed in 254a776
PostSelector has an excludeTree prop, that excludes the tree originating from a post ID.
We were passing it correctly, but somehow when updating the post without reloading the page, the current page leaked into PostSelector.
I've basically extended upon the existing logic, passing the excluded ID into the inner logic of PostSelector and directly checking upon it to explicitly exclude it (and its children too, for free) from the displayed tree.

As soon as I toggle the switch in the category modal, I get errors:

@shaunandrews Addressed in 69b3494
I've updated the dialog to prevent validating the form when disabling the Top Level toggle.
I didn't want to do any major changes on the whole validation logic (which imho is a bit too aggressive).
The only thing that made sense to me was to disable it in this particular case, that should only happen when folks are creating a new category and disable the Top Level toggle to pick a parent category: in other words, disabling the Top Level toggle is just the first half of the user intent, so it's not the right moment to nag them.

the label on the switch is capitalizing "Category"

@shaunandrews Yup, mentioned it in the (rather messy) PR description.

Apparently, terms labels are returned by the backend and stored in state; for this reason they are capitalized (e.g. "Choose a Parent Category").
I'm not exactly sure what is the best way to work around this, though.

To be fair, I think that changing how the backend creates those string is a bit overkill (from a dev standpoint), as it would require checking every single place they are used.

Probably it makes more sense to find labels that work better with capitalized words.

Do you have a suggestion how to update the label? 🤔

the error at the bottom mentions the top level option being "unchecked"

@shaunandrews Oh I think I get it now: it's a toggle, so "unchecked" it's incorrect.
What would be the correct term?
"untoggled" and "not toggled" both sound quite weird to me.
"disabled" too doesn't feel right in this case. 🤔

akirk · 2018-04-20T13:26:13Z

client/blocks/term-form-dialog/index.jsx

+					{ isTopLevel && (
+						<span className="term-form-dialog__top-level-description">
+							{ translate( 'Disable to select a %(parentTerm)s', {
+								args: { parentTerm: labels.parent_item },


Could you add a comment with examples of what the variable parentTerm could be? i.e. examples for "term". Please add it also to the other translatable strings where applicable. Thanks!

Comments added in c1203ec

kwight · 2018-04-23T18:10:00Z

This all works great for me now, except for a regular jank caused by a disappearing placeholder at the bottom of the page list:

shaunandrews · 2018-04-24T17:41:31Z

I've updated the dialog to prevent validating the form when disabling the Top Level toggle.

I'm still seeing the error immediately after disabling the toggle.

Do you have a suggestion how to update the label?

Hmmm. There's nothing super easy to do it. You could add another element around the term labels and use text-transform: lowercase; To be fair, we're (Calypso/Automattic/WordPress) all over the place when it comes to capitalization and sentence case. I wouldn't consider this a blocker.

"disabled" too doesn't feel right in this case.

Disabled was going to be my suggestion. Why do you think is feels wrong here?

except for a regular jank caused by a disappearing placeholder

I see this, too.

Copons · 2018-04-24T22:11:09Z

I'm still seeing the error immediately after disabling the toggle.

@shaunandrews I'm pretty sure this should have been fixed in 5822b64, and I don't see any errors when disabling (tried this branch on Calypso.live right now). 🤔
It does validate the form when enabling the toggle, though.

regular jank caused by a disappearing placeholder

That's the normal PostSelector behaviour when rendered from scratch.
You can replicate it in the Post Editor, HTML view, when opening the insert link dialog.
Off the top of my head, instead of toggling its render we could toggle its visibility, but I'd have to check if it messes up with what post fields are saved.

kwight · 2018-04-25T16:09:33Z

I don't see any errors when toggling the switch, and there's no jank now due to an update somewhere – unless @shaunandrews has serious concerns, I'm a 👍

Copons · 2018-04-25T17:05:04Z

there's no jank now due to an update somewhere

@kwight I'm pretty sure the jank is still there, but you don't notice it because it's at the bottom of the overflow: hidden post selector. 😄

In other words, you only actually see the placeholder flashing if there are less pages than the post selector height (like in your previous example).
Otherwise, you might notice a little twitch in the selector scrollbar, caused by the placeholder disappearing.

shaunandrews

I no longer see errors!

Copons added the [Type] Bug label Mar 21, 2018

Copons added Pages Posts [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Mar 21, 2018

Copons self-assigned this Mar 21, 2018

Copons requested review from shaunandrews, davewhitley and a team March 21, 2018 12:37

a8ci18n reviewed Mar 21, 2018

View reviewed changes

Copons added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 22, 2018

Copons force-pushed the fix/ui/8513-new-category-parent-page branch from c777449 to 69b3494 Compare April 18, 2018 15:56

Copons force-pushed the fix/ui/8513-new-category-parent-page branch from 69b3494 to 225e5d1 Compare April 18, 2018 16:13

Copons added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Apr 18, 2018

samouri mentioned this pull request Apr 18, 2018

test circleci transform-class #24308

Merged

Copons added 5 commits April 20, 2018 14:13

Update parent category dialog

2d239e6

Update parent page selector

9ba3b51

Fix initial isTopLevel handling

03a030e

Never show the excluded page in PostSelector

58ce792

Only validate the new category dialog when enabling the top level toggle

5822b64

Copons force-pushed the fix/ui/8513-new-category-parent-page branch from 225e5d1 to 5822b64 Compare April 20, 2018 13:14

akirk reviewed Apr 20, 2018

View reviewed changes

Add i18n comments

c1203ec

kwight approved these changes Apr 25, 2018

View reviewed changes

shaunandrews approved these changes Apr 25, 2018

View reviewed changes

Copons 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 Apr 25, 2018

Copons merged commit d34015f into master Apr 26, 2018

Copons deleted the fix/ui/8513-new-category-parent-page branch April 26, 2018 16:20

matticbot removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Ready to Merge labels Apr 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Update parent terms and pages selectors look and feel #23496

Editor: Update parent terms and pages selectors look and feel #23496

Copons commented Mar 21, 2018

matticbot commented Mar 21, 2018

a8ci18n Mar 21, 2018

akirk Apr 20, 2018

folletto commented Mar 21, 2018 •

edited

Loading

kwight commented Mar 21, 2018

shaunandrews commented Mar 22, 2018 •

edited

Loading

gwwar commented Apr 13, 2018

Copons commented Apr 18, 2018

akirk Apr 20, 2018

Copons Apr 20, 2018 •

edited

Loading

kwight commented Apr 23, 2018

shaunandrews commented Apr 24, 2018

Copons commented Apr 24, 2018

kwight commented Apr 25, 2018

Copons commented Apr 25, 2018 •

edited

Loading

shaunandrews left a comment

Editor: Update parent terms and pages selectors look and feel #23496

Editor: Update parent terms and pages selectors look and feel #23496

Conversation

Copons commented Mar 21, 2018

Parent Term Selector

Notes

Parent Page Selector

Notes

Testing instructions

matticbot commented Mar 21, 2018

a8ci18n Mar 21, 2018

Choose a reason for hiding this comment

akirk Apr 20, 2018

Choose a reason for hiding this comment

folletto commented Mar 21, 2018 • edited Loading

kwight commented Mar 21, 2018

shaunandrews commented Mar 22, 2018 • edited Loading

gwwar commented Apr 13, 2018

Copons commented Apr 18, 2018

akirk Apr 20, 2018

Choose a reason for hiding this comment

Copons Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

kwight commented Apr 23, 2018

shaunandrews commented Apr 24, 2018

Copons commented Apr 24, 2018

kwight commented Apr 25, 2018

Copons commented Apr 25, 2018 • edited Loading

shaunandrews left a comment

Choose a reason for hiding this comment

folletto commented Mar 21, 2018 •

edited

Loading

shaunandrews commented Mar 22, 2018 •

edited

Loading

Copons Apr 20, 2018 •

edited

Loading

Copons commented Apr 25, 2018 •

edited

Loading