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 editable Permalinks #5414

Closed
wants to merge 39 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@schlessera
Copy link
Member

schlessera commented Mar 5, 2018

This is a refresh of #3418.

Current changes relative to the previous PR:

  • Changes the modal into a Popover element to match the most recent design mockup (size is not mobile-ready yet).
  • Change button styling.
  • Add basic focus management.
  • Add support for permalink_structure, properly detecting where to replace the %postname% or disabling editing as needed.
  • Add basic sanitization to show what the slug will probably end up being.
  • Add visual feedback (currently red font color) to show that the change has not been persisted yet.

Before continuing work on this, I have a few questions I'd like to discuss:

  1. How should the Popover be positioned (to match the padding to the title as in the design mockups) or sized (to roughly match the post width without breaking on mobile)? I'm not sure directly sizing/positioning it is a good idea.
  2. What is the best way to access the data in wpApiSettings.schema? Is direct access the way to go here, or are we missing a mechanism?
  3. The slug is updated through the EDIT_POST in its slug property. This means that it updates on heartbeat with drafts, and on "Update" button with published posts. This also means we need to display visual feedback about this state. How should this be communicated?
  4. I provided basic focus management: icon click => focus "Copy", "Edit" click => focus input, "OK" click => focus "Copy". This probably needs an a11y review.
  5. The permalink structure can of course contain other tags apart from the %postname%. These all need to be computable for the permalink component to do its job. I can of course build all of this logic inside of the permalink component, but maybe there's a better way to do this. Duplicating all of this server code on the client side is one way to solve this, another would be to do a single request to the server replacing all tags with values. What is preferable here?
@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Mar 7, 2018

I just went to test this and not totally sure what is going on. When I click link icon I get:

2018-03-07 at 13 50

TypeError: Cannot read property 'focus' of null at e.value (http://gutenberg-wordpress.test/wp-content/plugins/gutenberg/editor/build/index.js?ver=1520429778:11:97170) at e.value (http://gutenberg-wordpress.test/wp-content/plugins/gutenberg/editor/build/index.js?ver=1520429778:11:96941) at e.<anonymous> (http://gutenberg-wordpress.test/wp-content/plugins/gutenberg/editor/build/index.js?ver=1520429778:11:94528) at je (http://gutenberg-wordpress.test/wp-content/plugins/gutenberg/vendor/react-dom.min.3583f8be.js:48:469) at commitLifeCycles (http://gutenberg-wordpress.test/wp-content/plugins/gutenberg/vendor/react-dom.min.3583f8be.js:149:146) at b (http://gutenberg-wordpress.test/wp-content/plugins/gutenberg/vendor/react-dom.min.3583f8be.js:156:29) at t (http://gutenberg-wordpress.test/wp-content/plugins/gutenberg/vendor/react-dom.min.3583f8be.js:167:47) at x (http://gutenberg-wordpress.test/wp-content/plugins/gutenberg/vendor/react-dom.min.3583f8be.js:166:247) at batchedUpdates (http://gutenberg-wordpress.test/wp-content/plugins/gutenberg/vendor/react-dom.min.3583f8be.js:169:173) at cc (http://gutenberg-wordpress.test/wp-content/plugins/gutenberg/vendor/react-dom.min.3583f8be.js:26:56)

It also does not seem like the mocks in the issue. I would recommend we keep to the agreed design: #3418 (comment) and iterations after that.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 8, 2018

Thank you for working on this. I wish I could snap my fingers and make this be fixed in an instant, it's such an important feature, so I appreciate your work.

It's not working too well for me in this implementation though. When you add a new post that hasn't been saved yet, there's a link icon, there probably shouldn't be since with the post not even being saved it can't have a permalink:

screen shot 2018-03-08 at 08 48 14

Clicking on it before saving also brings an error:

screen shot 2018-03-08 at 08 48 24

If you save the post and click the link, you get a popup, nice, that's what we want!

screen shot 2018-03-08 at 08 48 43

We'll want to polish the visuals a bit, the link icon is a bit awkwardly positioned and doesn't work on mobile:

screen shot 2018-03-08 at 08 52 12

screen shot 2018-03-08 at 08 52 18

But in the name of shipping I would say it's okay to not focus too heavily on the visuals right at this point. As soon as this is merged, I can create a PR and polish things up, fixing the visuals.

Editing the permalink seems to work! Yay! But I'm seeing an issue where no matter what I rename the permalink to, it ends up with name-sanitized, instead of just name:

screen shot 2018-03-08 at 08 49 05

screen shot 2018-03-08 at 08 48 55

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Mar 8, 2018

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 8, 2018

To answer a few of your questions:

How should the Popover be positioned (to match the padding to the title as in the design mockups) or sized (to roughly match the post width without breaking on mobile)? I'm not sure directly sizing/positioning it is a good idea.

I think you're decently close in this PR, but like I mentioned before, probably best to focus on the functionality. I think I can pretty quickly whip up some CSS in a polish PR once this is ready to go.

The slug is updated through the EDIT_POST in its slug property. This means that it updates on heartbeat with drafts, and on "Update" button with published posts. This also means we need to display visual feedback about this state. How should this be communicated?

CC: @adamsilverstein because I noticed the word "heartbeat", and I believe you might have input on that.

@schlessera

This comment has been minimized.

Copy link
Member Author

schlessera commented Mar 8, 2018

@karmatosed:
When fiddling around with the final permalink setup (to make all possible permalink structures work), it seems I broke new posts again. I'll fix this again.

@jasmussen:
Ah, the -sanitized bit is a left-over from my tests, I'll fix that.

Regarding the mobile version: I didn't yet spend any effort on this, as I'm not sure how to handle explicitly managing the Popover component.

pento and others added some commits Nov 10, 2017

Reintroduce "Copy" button next to permalink
The removal seems to have been unintended.
Adapt the buttons to share the same styling
Note: the `<Button>` component does not add the `button` class by default, so I added it manually. See #4638
@schlessera

This comment has been minimized.

Copy link
Member Author

schlessera commented Mar 8, 2018

@jasmussen I changed the code to disable the button on new posts. As soon as a first draft has been saved (even when it is through the heartbeat), it immediately gets enabled.

schlessera added some commits Mar 8, 2018

@schlessera

This comment has been minimized.

Copy link
Member Author

schlessera commented Mar 8, 2018

@jasmussen, @karmatosed Do you have any suggestion on how to provide visual feedback for now on the "tentative" state of the URL? Right now, it is changed to be red, which is obviously not a good final state.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 8, 2018

Do you have any suggestion on how to provide visual feedback for now on the "tentative" state of the URL? Right now, it is changed to be red, which is obviously not a good final state.

Can you elaborate on what this is? This is the "suggested" slug before it's been explicitly customized or published, yes?

If yes, then I would use the current WordPress editor pattern. No reason to highlight the state, in other words, I feel.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 8, 2018

Just tried the branch now, and it feels like we're WAY closer to a solid state. Getting no errors, and things seem to be working.

One thing though, on this test system I don't have fancy permalinks enabled, I just have ?p=223. I get this:

screen shot 2018-03-08 at 14 08 06

("gutenberg" is my localhost, so that part is correct, but it'd be nice if it would show the ?p=223 too, even if ineditable)

@schlessera

This comment has been minimized.

Copy link
Member Author

schlessera commented Mar 8, 2018

Can you elaborate on what this is? This is the "suggested" slug before it's been explicitly customized or published, yes?

Basically, we can only go so far in replicating the slug validation on the client side. In the end, it will need to be checked by the server against the database content. This might result in changes, like adding a numbered suffix (-2) to make it unique.

What is shown after clicking on "Ok" is only a best guess, until we persisted that change on the server side and got back the definitive slug.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 8, 2018

Oh I see, thanks for explaining.

In the end, it will need to be checked by the server against the database content. This might result in changes, like adding a numbered suffix (-2) to make it unique.

Would it be technically possible — not necessarily as part of this PR, just a potential enhancement in the future — to do that database check as soon as you press OK?

Here's how I'd dream of it working.

  • Let's say I have a post with the slug testing, already published
  • I write a newpost, save, edit the slug, type in testing again
  • When I press "OK" or Enter, either before the textfield disappears and turns into text, or shortly after, the slug would simply be corrected to testing-2

If that were possible, we might even look at showing a little notice on the screen, "A post with that slug was already published" etc etc.

If not possible, I still think we probably don't need to ambiguate with a color or icon or anything. The template, as far as I'm concerned, is the current WordPress editor as the first step, and simply replicating that functionality.

@schlessera

This comment has been minimized.

Copy link
Member Author

schlessera commented Mar 8, 2018

Would it be technically possible — not necessarily as part of this PR, just a potential enhancement in the future — to do that database check as soon as you press OK?

I think that modifying the permalink is tightly integrated with saving posts. Every save of a permalink would generate a new post revision.

schlessera added some commits Mar 8, 2018

@schlessera

This comment has been minimized.

Copy link
Member Author

schlessera commented Mar 8, 2018

("gutenberg" is my localhost, so that part is correct, but it'd be nice if it would show the ?p=223 too, even if ineditable)

This should be fixed now.

Also, I added actual data for all the date-based tags in the permalink structure, as well as the post ID.

The following tags are currently not properly implemented, as they cannot easily be fetched through the post data:

  • %author%: We already have the author ID, but we need the slug.
  • %category%: This probably needs a lot of additional logic, including a REST API request.
@schlessera

This comment has been minimized.

Copy link
Member Author

schlessera commented Mar 8, 2018

@karmatosed, @jasmussen

It also does not seem like the mocks in the issue. I would recommend we keep to the agreed design: #3418 (comment) and iterations after that.

These mocks are from 26 Nov 2017. The initial implementation I did was based on these. After it was merged, @karmatosed objected that it was straying too far off the design. This led me to find the following mockups, that are from Jan 15. 2018: #1285 (comment)

The current implementation is based on these mockups, that are said to be "updated".

I'm unsure now what the actual design direction should be for the above component.

I have to say that the Popover version I implemented here does seem like it has the following benefits:

  • It is an established UI component that gets reused in multiple places, instead of a random div with some styling. This probably decreases the overall learning curve.
  • It handles a lot of the UX automatically, like hiding when you click out of focus. This reduces duplicated logic and makes the experience more consistent.
  • It offers better collaboration with the other UI elements on the screen.
@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 8, 2018

The current implementation is based on these mockups, that are said to be "updated".

Ah yes, I suppose there's been some miscommunication there. Tammie I would suggest that #1285 (comment) are the ones we want.

However to a larger point — and I feel like this is implied in Tammies comment — the UI we can always fix, there are a bunch of us here to help. What is harder to fix, and where we need your expertise, is in getting permalink editing to work, and again, grateful for your help here.

Edit, to put it differently: you could've done zero UI work and just made the links editable, and we could've explored enhancements like a popup separately.

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Mar 8, 2018

This really seems design wise to have wondered from the agreed path. @jasmussen's design was just to see, not agreed to implement. I would like to go back to having the signed off design made and we can then iterate.

When I commented it had strayed there were a number of things that hadn't been done that we'd agreed experience wise. It wasn't that we should go with a different design. I feel that's where this confusion came in, it was referring to a branch.

To be clear: #3418 (comment) has the final version we should be going with. I understand this has gotten confusing but please revert back to that @schlessera. What was being suggested with the popover was merely a 'this could be iterated', we would do that after.

@karmatosed
Copy link
Member

karmatosed left a comment

This is not as the agreed UI, please make sure it is per this design #3418 (comment)

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 8, 2018

In my excitement that we will soon be having editable permalinks, I completely forgot that a more recent design existed in #3418 (comment). I'm fully behind that design. As with all things, things can be revisited in the future, for example with a last minute checkup in the Publish confirm sidebar. But Tammie is right — that was not the conclusion to the discussion, so you can disregard my comment here.

Even though it's not late, it's already been a long day, and I skimmed things a bit too quickly. I clearly need sleep. My apologies to Tammie for not recognizing the latest mockup, and to you as well for not catching that sooner. Your work is still very much appreciated, but we should course correct on the UI.

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Mar 8, 2018

No apologies needed @jasmussen, this is also a symptom of there being a new PR, all context in old one was lost and that has lead us quite away from the path. Onwards :)

@schlessera

This comment has been minimized.

Copy link
Member Author

schlessera commented Mar 8, 2018

I completely understand that decisions that were taken need to be respected, to avoid chaos, and having everything split between multiple issues/PRs certainly doesn't help. I'm unhappy to revert, as I think the PR will be worse off, but will submit a commit to get back to the old mockup as soon as I find the time.

However, before continuing to work on this, I'd like to ask how to detect what was decided on, to make sure I don't waste people's time (and my own) any further. Basically, is there something that denotes a given mockup as "this is the one"? And if not, can we introduce something like that? I've only started contributing to the codebase more recently, so I might just have missed important information in that regard.

Just to clarify why I'm still not clear on this, I'll try to explain what I did to identify the mockup to use, maybe it helps refine the process:

  • The mockup in #3418 (comment) (A) is not the latest one. There's also no mention anywhere about there being a decision. There's only a question at the end: "@jasmussen what do you think of this as a suggestion?", which makes me assume that at least at that point, the mockup is only a suggestion and nothing was decided yet.
  • @jasmussen has later submitted a different mockup in #1285 (comment) (B), and that mockup seemed to solve some of the problems that were still plaguing the mockup in (A), like the fact that the permalink box was completely unusable as soon as you hit the Save button the first time. As I couldn't detect any finality in (A), I assumed (B) was the replacement to deal with the identified issues in (A).

Maybe the above helps avoid future confusion.

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Mar 9, 2018

@schlessera thanks for the reply, let's work through it a little and hopefully get back on track.

I'm unhappy to revert, as I think the PR will be worse off, but will submit a commit to get back to the old mockup as soon as I find the time.

By reverting I meant just that, lets go back to the original PR design, so let's do that. Apologies if the revert term got you thinking something else was meant.

However, before continuing to work on this, I'd like to ask how to detect what was decided on, to make sure I don't waste people's time (and my own) any further. Basically, is there something that denotes a given mockup as "this is the one"? And if not, can we introduce something like that? I've only started contributing to the codebase more recently, so I might just have missed important information in that regard.

Let's break this down a little. There were a few points of failure in communication we can fix. Firstly, just by having this PR away from the history caused issues. In future maybe we can link back to the history more. The designs and conversation was there.

Our process has worked pretty solidly to this point, discuss in an issue, then a PR and commit. This is the first time something like this has happened and gone so far off track. That's good though as means we can be clear going forward and not have it happen again.

The mockup in #3418 (comment) (A) is not the latest one. There's also no mention anywhere about there being a decision. There's only a question at the end: "@jasmussen what do you think of this as a suggestion?", which makes me assume that at least at that point, the mockup is only a suggestion and nothing was decided yet.

It is the latest in relation to that PR which was what you were working on. Also the other one is closed, which is a good indication we're not working on that right now design wise. My comment on that mock indicates:

Does that need a redesign when we're working on that for now @jasmussen? My suggestion would be get the editable in and then we can iterate. This feels a better approach to this.

To which @jasmussen agrees:

Just to clarify — we need to make the permalink editable first. ANY improvements to the design can and should happen after that, and aren't even as high priority.

Then it closed, which really was an indication we were moving onto focusing on the current PR that Gary had created.

that mockup seemed to solve some of the problems that were still plaguing the mockup in

I think it's important to get the design we are working on in before we judge it as having problems. It actually avoids some added by a popover. From device issues to usability, there are a number of concerns with the popover, none more prominent than it was just a first sketch, no exploration beyond that.

To be clear, both myself and @jasmussen do not see this as a final design. It was done to solve issues and then we always planned to iterate. It will end up likely not any of the designs seen currently, we're researching that. However as this is such an asked for and used feature we just want it in, with the design that is as close to the existing one as possible. That works across all devices easier and was in the original PR.

I hope this helps clarify.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Mar 9, 2018

I tend to always want to move fast, and so as I tested your PR I failed to recall the past discussion on designs. In this case, in other words, I moved too fast. Sorry about that.

The present UI for permalinks (even sans editing), is — as @karmatosed above also notes — a suboptimal user experience. As has been mentioned in many places, the slug is important for when you're writing pages, it can be important for SEO, and if it only appears when you select the header block first, it's too hidden. It's arguably too hidden, also, behind a link button such as I have mocked up way back when. Future enhancements are definitely needed, but as for what they might look like is still not completely clear. One enhancement we could at the very least do, is surface the permalink UI as part of the pre-publish experience. But this should not stop us from shipping the editability.

As a side note, everyone should always feel welcome to ping anyone on the team directly, ask questions in #core-editor in the Core Slack, if anything is unclear. Thanks again for working on this.

@schlessera

This comment has been minimized.

Copy link
Member Author

schlessera commented Mar 10, 2018

By reverting I meant just that, lets go back to the original PR design, so let's do that. Apologies if the revert term got you thinking something else was meant.

Ah, no worries, I understood what you meant. What makes me "unhappy" is that the PopOver is a component that deals with all sorts of UX behavior in a centralized way and can be improved outside of the Permalink component. What we return back to now is basically a div with hardcoded behavior isolated within the Permalink, so it will need many further changes down the road, irregardless of whether that kind of behavior was already fixed elsewhere. But that's not a big issue, only a general dislike for me as a developer.

Thanks for taking the time to discuss my concerns, it is much appreciated.

@pento pento referenced this pull request Mar 23, 2018

Merged

Add Editable Permalinks #5756

3 of 3 tasks complete
@pento

This comment has been minimized.

Copy link
Member

pento commented Mar 23, 2018

Due to the size of this PR, #5756 is a fresh attempt at editable permalinks, based on @karmatosed's design.

@pento pento closed this in 4c90150 Apr 15, 2018

@pento pento deleted the add/1285-editable-permalinks-2 branch Apr 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment