Editor: Implement MVP for Post Locking (Story Locking) #6497
Conversation
spacedmonkey
added
Type: Enhancement
|
Size Change: +2.93 kB (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
Yes, it's an acceptance criteria, see #3126.
Yes, it's an acceptance criteria, see #3126.
Let's look into tracking towards the end of the work on this feature. Perhaps in a separate PR. |
assets/src/dashboard/icons/lock.svg
Outdated
| <!-- Generator: Sketch 60.1 (88133) - https://sketch.com --> | ||
| <title>Artboard</title> | ||
| <desc>Created with Sketch.</desc> |
There was a problem hiding this comment.
Where is this icon coming from? 🤔
There was a problem hiding this comment.
There was a problem hiding this comment.
Let's use lock_closed.svg from the design system instead of duplicating files.
There was a problem hiding this comment.
I used this icon, but had to do some series hacking around to make it look good, as the icon has one space around it
includes/Story_Post_Type.php
Outdated
| if ( ! function_exists( 'wp_check_post_lock' ) || ! function_exists( 'wp_set_post_lock' ) ) { | ||
| require_once ABSPATH . 'wp-admin/includes/post.php'; | ||
| } |
There was a problem hiding this comment.
Why shouldn't this function already exist? 🤔
We're already in the admin and in the post editor, it should exist at this point, no?
If it's for tests, we could load the file in the test suite instead.
There was a problem hiding this comment.
I do this elsewhere in the core, just being overly careful. I can remove if it really bothers you. Just worried this function might be called out of context for some reason...
| ); | ||
|
|
||
| // When async call only if dialog is true, current user is loaded and post locking is enabled. | ||
| const doGetStoryLock = useCallback(() => { |
There was a problem hiding this comment.
This function is really hard to grasp as it is not very readable and does a lot of things at once.
It might be beneficial to extract parts of it into an effect or something, or splitting this up into two functions.
Can be done in a follow-up PR of course, just wanted to mention it.
A downside of the current approach here is that we first have to do GET request to get the current lock and then a POST request, which seems a bit wasteful (compared to just 1 request to refresh an existing lock)
There was a problem hiding this comment.
I think this whole file is a little bit too big. I was thinking about breaking this into a hook / provider. We can do this later, as you say.
| * @param Array $lock Lock array. | ||
| * @param WP_REST_Request $request Request object. | ||
| */ | ||
| return apply_filters( "rest_prepare_{$this->post_type}_lock", $response, $lock, $request ); |
There was a problem hiding this comment.
Should we prefix this with web_stories_?
There was a problem hiding this comment.
This just follows core's pattern. I can change if you want.
Co-authored-by: Pascal Birchler <[email protected]>
Co-authored-by: Pascal Birchler <[email protected]>
swissspidy
changed the title
spacedmonkey
force-pushed
the
try/post-locking
branch
from
1edbe1f
to
935bb08
Compare
Codecov Report
@@ Coverage Diff @@
## main #6497 +/- ##
===========================================
+ Coverage 64.58% 80.05% +15.47%
===========================================
Files 1041 1196 +155
Lines 16985 22014 +5029
===========================================
+ Hits 10970 17624 +6654
+ Misses 6015 4390 -1625
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Context
Summary
A very basic implementation of post locking in the editor.
Please note, the code is very messy and needs a lot of tidying up. The idea was just to get this thing working.
Relevant Technical Choices
Some of the fields adding to the settings object, maybe loaded from other places. But as the post locking is loaded very early.
No new endpoint was created, I used extended existing endpoints.
User data is loaded via an embedded REST API request.
Post lock data can not be preloaded as it caches.
To-do
Should we implement delete lock on navigation away from story editor?
Should this feature be behind a feature flag?
Should the links be tracked?
User-facing changes
Screen.Recording.2021-02-24.at.16.10.30.mov
Testing Instructions
QA
This PR can be tested by following these steps:
UAT
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
Does this PR change what data or activity we track or use?
Does this PR have a legal-related impact?
Checklist
Type: XYZlabel to the PRFixes #3126