Quick Draft: Leverage REST API endpoints

[adamsilverstein]

If the REST API content endpoints were in core, how would we rebuild core features (like Quick Draft) to use it? I am opening this ticket to track work on converting the Quick Draft feature to using the REST API.

The quick draft feature is a meta box on the Dashboard for creating draft posts:

Quick Draft currently uses a simple form to post to post.php and create a the draft. The goal of this ticket would be to switch this action to JavaScript and the REST API. In addition, we would switch the list of recent drafts below the form to load via the API and render dynamically as well as adding a progress indicator and confirmation/error message. Note that the form is already hidden if JavaScript isn't available.

Aaron Rutley has already coded a POC version here: ​https://github.com/AaronRutley/quick-rest-draft

[kraftbj]

One thing I started noodling on awhile back was Quick Draft to save progress. The use case is similar to the post editor—if you leave your wp-admin open too long, the nonce can expire, and you'll lose your post if attempting to save. I'm not sure if it makes sense to add this as an unrelated enhancement to save to the local browser storage or save a draft to the db.

[adamsilverstein]

A Quick update here: I started work on this, still needs some more testing and polishing then I will submit as a patch. In the mean time, here is what I have so far: ​https://github.com/xwp/wordpress-develop/compare/master...adamsilverstein:ticket-38342

[aduth]

If the REST API content endpoints were in core, how would we rebuild core features (like Quick Draft) to use it?

After chatting with @adamsilverstein , my take on this was to do a more thorough overhaul of the existing Quick Drafts, leveraging the ​Backbone.js API client proposed to be merged with the content endpoints. I was interested in seeing what it would look like to have Quick Posts operate exclusively on data from the posts endpoints, bootstrapping data on the initial page load using rest_do_request and maintaining a Backbone collection of drafts in browser state.

WIP: ​https://github.com/aduth/wordpress-develop/compare/master...update/trac-38342-api-quick-draft

[adamsilverstein]

Looking good!

Continuing work on this here, i'll give you commit there @aduth so we can collaborate more easily ​https://github.com/adamsilverstein/wordpress-develop/commits/update/trac-38342-api-quick-draft

Contributions & feedback welcome, work in progress.

[adamsilverstein]

WIP patch for testing

[aduth]

The above patch is more-or-less functionally complete. The experience should feel much the same as it is currently.

Some thoughts and pain points encountered in the course of implementing this:

• Bootstrapping data helps in displaying recent drafts more quickly, but there's still a delay for as long as it takes for the dashboard script to load. I added some placeholder elements to give the illusion of incoming content and reduce the jarring effect of items suddenly appearing.
• It's still not clear how we'd access certain data from JavaScript. Content such as: formatted dates, links where filters are run server-side (e.g. edit link), truncated content, etc. Script localization, porting functions to JavaScript, and newer browser features like ​Intl.DateTimeFormat (with appropriate fallbacks) can help achieve these. Other content might require filters on the API endpoints return values themselves.
• Quick Draft already requires JavaScript anyways, but it's worth consideration for if and how content should be rendered "isomorphically" (i.e. complete markup sent from the server, but thereafter controlled in the browser). This could help remedy the need for placeholders as the script loads, but the state of tooling is such that the logic would need to be duplicated to implement (rendering in PHP/Backbone, querying from REST API/WP_Query).

[timmydcrawford]

Do we need to handle the possibility of the core endpoints being disabled on a site in this change? I guess that question is just a larger general question I had on the topic of swapping out existing use-cases in wp-admin to use the API.

[aduth]

@timmydcrawford : It appears it is possible to filter out the posts endpoint routes:

​https://github.com/WordPress/WordPress/blob/e4a7c0a/wp-includes/rest-api/class-wp-rest-server.php#L701-L711

Unlikely as it might be, would be a good idea to account for this case by checking whether the required endpoints are registered before displaying Quick Drafts.

[jnylen0]

The consensus on #38339 is that disabling the core endpoints is a bad idea, which makes sense to me.

We'll need to disable/restrict a couple of things in WordPress.com, but IMO it's ok for core to not have to worry about this too much.

[joehoyle]

Just a quick note on this patch before I forget, the use of 'filter[post_type]' => 'post', is not required here. This is looking cool, though!

[adamsilverstein]

A few more questions that came up when working on #38343:

• How do we support filters in existing callbacks (for backwards compatibility) or if we want to add them PHP side for a future feature? Could we start leveraging JavaScript filters?
• What is the best way to add data that is not part of the standard responses?
• How do we handle endpoints/REST API being deactivated for core features?

[helen]

Milestoning into 4.7 as an enhancement because A) some kind of core usage of REST API endpoints was requested, and B) I would like to see at least the questions/concerns addressed before beta 1, because I think those are important things core needs to answer for if we're shipping these endpoints.

[joehoyle]

I think this is looking great - on the questions, this is broadly my opinion, however I'll also ask someone else from the REST API team to weigh in.

Bootstrapping data helps in displaying recent drafts more quickly, but there's still a delay for as long as it takes for the dashboard script to load. I added some placeholder elements to give the illusion of incoming content and reduce the jarring effect of items suddenly appearing.

I think the effect is very cool, and I don't really see this as much of a limitation. Right now, it's true that it makes a while to get your admin scripts bootstrapped because the admin loads, well, a lot of stuff, and by doing it this way we are basically accepting it's going to load last, but I think that's ok.

It's still not clear how we'd access certain data from JavaScript. Content such as: formatted dates, links where filters are run server-side (e.g. edit link), truncated content, etc. Script localization, porting functions to JavaScript, and newer browser features like Intl.DateTimeFormat (with appropriate fallbacks) can help achieve these. Other content might require filters on the API endpoints return values themselves.

So, broadly speaking I think we should be looking at shifting a lot of this stuff to the front-end. This like truncated content / excerpts, localized dates etc. In a REST API driven world, content transformation etc is expected to live much more on the client that on the backend. So, yes - I think we should be looking at solving things like date formatting via JavaScript that can then be used in the admin anywhere.

Quick Draft already requires JavaScript anyways, but it's worth consideration for if and how content should be rendered "isomorphically" (i.e. complete markup sent from the server, but thereafter controlled in the browser). This could help remedy the need for placeholders as the script loads, but the state of tooling is such that the logic would need to be duplicated to implement (rendering in PHP/Backbone, querying from REST API/WP_Query).

I'm not sure what the "official" stance on JS compatibility is. Does / should the wp-admin function without JavaScript? I don't think it does very well right now (e.g. how do I insert an image into a post) so I think we may be ok to say that we can require the use of JS for a feature. Given that, and the nature of the WP Admin application, I don't see isomorphic rendering as that important. I was even a little surprised to see this patch does a rest_do_request for the post list, rather than on-load. It makes sense as long as the Quick Draft is always shown, if I hide it in screen options for example, then really we shouldn't be preloading that data (sorry if that case is actually handled), in which case those logic checks, like "is this box actually shown, or is it hidden with css" is much easier to answer from the javascript context, and we can be much smarter about when to load data.

@adamsilverstein:

How do we support filters in existing callbacks (for backwards compatibility) or if we want to add them PHP side for a future feature? Could we start leveraging JavaScript filters?

If this is for backwards compat, I don't see a case for the JS filters - the PHP filters side of things is a little more tricky. For example, as we replace admin-ajax stuff, those used to be filterable in a different way. Given these filters can be quite esoteric I'm not sure that we need to support admin-ajax filters. However, I'm sure there are situations where we _will_ need to support PHP filters, (like prepare_attachment_for_js for example) and in those cases we'll have to do the usual "try and work out the most elegant hack to fire them with the same things as before" type thing that happens whenever we re-build a feature in WordPress. That's not always an easy thing to do, and I don't see a great fix-all solution there, given how different every filter can be.

What is the best way to add data that is not part of the standard responses?

This _can_ be done with adding meta, register_rest_field etc, however I think this should be looked at on a case-by-case basis. If there is data that we are outright missing from the API, let's try to get that into the API. If on the other hand, it's something presentation, that _did_ previously live at the PHP-send-it-over-the-wire-with-the-data level, we should take a fresh look at how to go about achieving the same result from a JavaScript / Front end centric point of view. I appreciate the REST API is a lot more "pure" it it's approach, and that can be frustrating coming from a PHP/WP world where we just put any data we need _anywhere_ to get the job done. I don't want to discard that pragmatism, but I think we should be striving to keep things "RESTful".

[kadamwhite]

Regarding the question, "How do we handle endpoints/REST API being deactivated for core features?", as noted above the consensus on #38339 does seem to be towards preventing de-registering the API endpoints. Since we need to leverage cookie auth for the post editor anyway, and requiring authentication is the most likely way in which the REST API would be locked down, I believe it's safe to put this concern aside. I think Joe touched on the rest.

[dd32]

Personally, I think in the event that the JS attempts to use the REST API and finds it unavailable, it should gracefully accept that situation and display an error to that effect. "Unavailable, something horrible has happened to your WordPress install!" (better worded of course).
I wouldn't worry about a fallback or detecting if it's available before initing the module - just as long as their draft content is still available for them to copy-paste to save via the normal editor.

[aduth]

Latest patch includes:

• Remove data bootstrapping server-side, instead fetching on client. This hadn't accounted for user dismissed panels, which is technically possible to account for on the server, but simpler to delegate to client (with caveat being there's a neglibily longer delay in drafts being shown)
• Better compatibility with ​non-ES5 browsers, e.g. using Underscore.js equivalents in place of Array#map and Function#bind
• Add inline comments

​Full commit history on GitHub

[aduth]

Latest patch resolves outstanding IE8 issues, particularly around parsing dates.

Implementing these changes highlighted another recurring pain encountered throughout the process, which is that the _gmt date fields from the REST API are not reliable. This issue is not specific to the REST API, as the fields themselves are only set to the database for a post in particular circumstances (ticket:5698#comment:14). If a post is created via the REST API without a date explicitly set, both date_gmt and modified_gmt will be null (0000-00-00 00:00:00 in the database). This inconsistency forces us to use date instead, which is offset by the site's timezone setting. Working with dates -- particularly those with timezone offsets -- in JavaScript, is very difficult and error-prone. A library like ​Moment.js is almost essential if this is to become more common. Otherwise, it would be nice if we could be guaranteed that the REST API would return valid _gmt dates in all cases.

[adamsilverstein]

This is looking really good @aduth, thanks for putting so much work into this feature!

The issues you raised around Dates are important and were a serious pain point when developing the wp-api Backbone client - in particular the new draft post_status case where the {}_gmt dates are returned as null from the api. I think the timezone offset is adequate here. I'd love to see us include Moment.js and possibly Moment-Timezone for better date support, these are great libraries.

One possible issue with the API returning _gmt when the database doesn't have a gmt date is that sending back that same data (when saving the post again) will cause unexpected results.

I wonder how we can address the underlying issue with null _gmt dates in the database and still cover the goal of #5698?

[rachelbaker]

@aduth It looks like your patches (even 38342-4.diff​ ) include code-style only modifications to lines in dashboard.js. Can you remove those from the patch?

[adamsilverstein]

@aduth This works great in my testing! A tiny bit of feedback:

• I love the 'shell' animated loading indicator you added, however the shell always shows 4 drafts and if you have fewer it looks odd when loading. Maybe only show one here or possibly no shells at all? probably not worth the effort of a count query to determine the actual number of drafts.
• Ideally we should add a hide-if-no-js class to the meta box, since the feature won't work without JS. I think we have to do that via a filter.

[aduth]

Remove unrelated style updates to dashboard.js, adhere to spacing exceptions from JS guidelines

[rachelbaker]

@adamsilverstein I am interested in your thoughts around the wp_dashboard_recent_drafts() function. Looking at 38342-4.diff​ the echoing output here seems out of place in a php function. Any concern about the back-compat break of losing the 'dashboard_recent_drafts_query_args' filter?

[aduth]

I love the 'shell' animated loading indicator you added, however the shell always shows 4 drafts and if you have fewer it looks odd when loading. Maybe only show one here or possibly no shells at all? probably not worth the effort of a count query to determine the actual number of drafts.

Yeah, though it's a similarly jarring effect when there's no initial placeholders, then replaced with drafts after the API request finishes. I think the best approach would depend on what's most common case: do most sites have many drafts? A couple? None? I suspect this might be "none", but would be best to back up by stats.

Ideally we should add a hide-if-no-js class to the meta box, since the feature won't work without JS. I think we have to do that via a filter.

Good call. This is already assigned to the Quick Drafts form and list heading, but I think it should apply to the entire meta box. I imagine with the latest patch, placeholders will be shown forever if JavaScript is disabled.

[adamsilverstein]

Replying to rachelbaker:

@adamsilverstein I am interested in your thoughts around the wp_dashboard_recent_drafts() function. Looking at 38342-4.diff​ the echoing output here seems out of place in a php function. Any concern about the back-compat break of losing the 'dashboard_recent_drafts_query_args' filter?

Good point @rachelbaker, I missed the removal of the filter. I think we would need to add that existing filter via a callback on a rest api filter, similar to what I am doing for Press This.

With the goal of achieving something to demonstrate best practices of working with the API endpoints in core, I would like to propose we start with a much simpler patch that only seeks to replace the post action of the form with an API callback, then I think we can continue refining the draft post list part in 4.8, and trying to address some of the issues that we have run into with dates and backwards compatibility.

[adamsilverstein]

38342.2.diff​ is a much simpler patch that only replaces the post action of the quick draft form. This is pretty close to what I proposed originally, I extracted it from earlier in the commit history. I'm going to do some testing, capture a screencast and see if it can be improved.

[adamsilverstein]

Now that I'm testing 38342.2.diff​ I see why @aduth took on the full rewrite: the recent draft list isn't getting updated with the new drafts! I'm going to try to work up something simpler, however as we are hitting the beta deadline, we may not make it into this release.

Regardless of when/if this is merged to core, the process of working on this ticket has been quite valuable, raising some important questions and discussions. While we didn’t find anything insurmountable, we did encounter several pain points that we will want to address as we increasingly leverage the API in core.

[helen]

Not going to make it for 4.7, but let's definitely keep on this for 4.8 :)

[adamsilverstein]

38342.3.diff​

• Add a count to the shell construction to the shell shows a matching number of slots as the displayed drafts
• don't remove existing wp_dashboard_recent_drafts function or ajax callback, keeping patch smaller and in case they are being used by developers; before removing we should check for usage, then deprecate the function
• build and return the js template instead of echoing it out directly in the function

working on adding the missing filter.

[adamsilverstein]

In 38342.4.diff​

• Add support for the legacy dashboard_recent_drafts_query_args filter that currently runs in wp_dashboard_recent_drafts. The filter is applied to both the shell count query and the rest_api callback query, enabling developers to (continue) modifying the default behaviour of the recent drafts list. Existing filter usage should continue to work.

For example, I tested increasing the number of drafts shown in the list to 10 using the following code in my theme functions.php, similar code could modify the query to change the defaults, for example adding a cpt or showing drafts from all users:

// Show 10 drafts in the recent draft list in the quick draft dashboard widget.
function filter_the_draft_list( $query_args ) {
	$query_args['posts_per_page'] = 10;
	return $query_args;
}
add_filter( 'dashboard_recent_drafts_query_args', 'filter_the_draft_list' );

Here is a screenshot of the result:

[adamsilverstein]

@rachelbaker I would appreciate your feedback here on the approach used to reintroduce the legacy filter, in particular would like feedback on how I added it to the api callback, how I limited the filtering to quick post queries and where I put the code.

Thanks!

[aduth]

Nice work on latest patch. Some thoughts as I reviewed:

• Is wp-includes/rest-api.php the best place to add the legacy filter? I might imagine there could be many of these filters as more queries are replaced with client-side API requests.
• Speaking to this point, should there be more of a standard around how we handle overrides for specific admin requests? Maybe a standard header named to reflect requests for a particular admin context?
• Despite pointer comments suggesting otherwise, there does not appear to be any documentation for the dashboard_recent_drafts_query_args filter.
• I'm a bit on the fence with the server-side query for post count. Part of me can't help but feel if we're querying posts server-side to render placeholders, we may as well be rendering them too. I realize fields / no_found_rows make the query more efficient than querying the full objects, but it almost feels an admission that a fully JS solution isn't quite sufficient on its own.
• Could we avoid duplication of the default fetch parameters in the client? Maybe a standalone function that returns the query arguments that can be called from the API override or script localization?
• More filters that we're still not accounting for: admin_url (for edit link), get_the_time (timestamp)

[aduth]

To address remaining concerns with recreating filters and text formatting on the client, the latest patch takes an approach of adding a new REST API endpoint for Quick Drafts, extending WP_REST_Posts_Controller in a new wp-admin/v1 namespace, including only attributes relevant for display of Quick Drafts and allowing creation by the same endpoint. Benefits of this include having full control to customize the endpoint request and response structure, downside being significantly more boilerplate to support the new endpoint.

Changes:

• Create new WP_REST_Quick_Drafts_Controller endpoint for listing and creating Quick Drafts
• Re-render draft form on save attempt to reflect accurate entry validity
• Limit list to four entries even after having created new drafts
• Remove now-unused wp.formatting.trimWords JavaScript implementation
• Bump version on dashboard script to ensure cachebust

[joehoyle]

Just getting stuck into to helping out here, after reviewing these patches, I think staying with the direction of @adamsilverstein's latest patch is the direction we should continue. @aduth I think the REST API custom endpoint is the wrong approach here - this is the antithesis of what the REST API is trying to solve. If we _need_ a custom endpoint just to build quick drafts, we have much bigger problems.

We should be moving more to JS / front-end, that's what the whole separation of the API is meant for. Back end is for the resource logic and storage, front-end becomes the place to do presentational things. When switching to the REST API, there's no doubt a new class of problems to solve crop up, that are not present in our server-side rendering world. I can definitely understand the want to pull render logic back to the server, but I think we can do better, on the front end. This opens up much more possibilities even in dates alone, it's possible to do dynamic / live updating relative dates etc if we have that logic on the client.

I think we are identifying some great pain points here, but the solution is to push forward and solve them on the client, paving the way for other parts of the admin to take this approach, and also help others with good examples. Going with a custom server controller I think is the opposite of that.

It seems we have a couple of unsolved problems to solve:

1. Pre-render placeholder counts before communicating with the API
2. Date formatting, though it actually looks like maybe @adamsilverstein did solve this one?

I think it would be nice to build the template more on the front end, to do that it seems primarily we'ed need:

1. Edit Links on the resources via the API
2. Translation strings passed via wp_localize_script.

On "Pre-render placeholder counts before communicating with the API", I don't actually think we should worry about this. The place holder does not need to be accurate. Take Facebook, Slack etc - the placeholder style like this is never the right amount, it's just saying "expect some content to load in here", so - I think we should remove the count from the backend to simplify this.

[joehoyle]

Added a new patch building on @adamsilverstein's latest one with:

1. Fix date/timezone offsets by passing the offset via wp_localize_script.
2. Remove the WP_Query for the place holder count, instead hardcoding to 1.

For the filter, I think the solution here is fine - but we might want to move it somewhere else, having said that we can't put it anywhere in the admin files of course as the API request is outside of this context, so I don't think it matters too much.

I'm also happy with formatting the date via Intl (see caniuse.com/#search=Intl) as it's fairly well supported, and luckily, the fallback works well given we only need the date.

I think this is looking actually very complete at this point - great work so far. I'd like to see if we can remove wp_dashboard_get_recent_drafts_js_template from being generated in PHP, and instead put it in a JS file, like our other backbone templates. I'll have a go at doing that next.

[joehoyle]

I added a new patch to load the item-quick-press-draft template in the footer, like we do with most other underscore.js templates. I also simplified the output function to just include id="quick-press-drafts" below the form, as we don't need the other PHP function now as wp_dashboard_print_recent_drafts_template is hooked to admin_footer

[jnylen0]

The latest file uploaded here (38531.8.diff​) is from a different ticket (#38531).

More generally, can we please fix this from earlier in the thread that hasn't been addressed yet?

If a post is created via the REST API without a date explicitly set, both date_gmt and modified_gmt will be null (0000-00-00 00:00:00 in the database). This inconsistency forces us to use date instead, which is offset by the site's timezone setting. Working with dates -- particularly those with timezone offsets -- in JavaScript, is very difficult and error-prone. A library like Moment.js is almost essential if this is to become more common. Otherwise, it would be nice if we could be guaranteed that the REST API would return valid _gmt dates in all cases.

If we can't fix the GMT issue and provide some clear recommendations for how to handle dates, certain edge cases around DST transitions are impossible to solve correctly, and we are strongly encouraging people to write broken code. Using GMT dates is the best practice for a reason: handling dates/times correctly is really really hard, and that makes it much simpler.

Also, it's fine to say "Date & time formatting is not handled in the API" (​ref) - I agree that this should be handled by the client. However, we should provide examples / functions / best practices for doing this. Otherwise we will end up with WP installs containing 8 different date libraries, several copies of moment.js, hand-rolled date code that only works on the site where it was originally written (until DST comes around), and so on.

[jnylen0]

Also, one more wrinkle that hasn't been discussed much yet: the need to use wp_localize_script to pass settings such as site offset to the client. This is an ok solution for clients that live within the admin, but:

• It's not very RESTful.
• It won't work for any client outside of the admin.

No doubt there will be more reasons to do this sort of thing in the future.

As mentioned in ​this docs PR, the current settings endpoint is not a good solution because it requires manage_options.

Plugins can write custom endpoints to expose the data they need, but this will lead to a lot of duplicated code and unnecessary extra API endpoints. More code, more surface area, more bugs.

There's a ticket about finer-grained access to the data in site settings (#38731). I think we should take a look because it's directly relevant to this effort.

[jnylen0]

In 38342.8.diff​:

<?php
if ( ! isset( $params['quick-draft-post-list'] ) ) { 
    return $response;
}

$response is undefined here - should be return $query_args;

[aduth]

When switching to the REST API, there's no doubt a new class of problems to solve crop up, that are not present in our server-side rendering world. I can definitely understand the want to pull render logic back to the server, but I think we can do better, on the front end.

Is this any different with the the rest_filter_quick_draft_query filter, though? And the filter alone does not account for modifications that would have occurred previously by the ​wp_trim_words and ​get_edit_post_link filters in the ​original template logic, so we're in this weird place where we preserve backwards compatibility for some but not all of the original implementation.

That said, I agree with your general premise that it would be best to leverage transformations of API data in the client, which is why I'd included implementations of wp.formatting.trimWords and Intl.DateTimeFormat in earlier patches. As we've seen here though, keeping 100% backwards compatibility with filters that would have previously run server-side will be difficult to achieve moving forward. I don't really have a solution to offer at the moment, and in retrospect would relent that my latest patch is probably a step in the wrong direction.

[joehoyle]

Also, one more wrinkle that hasn't been discussed much yet: the need to use wp_localize_script to pass settings such as site offset to the client. This is an ok solution for clients that live within the admin, but:

I agree this isn't ideal - I mentioned in #38883 that adding the offset to /wp-json/ along with url / name / description could be a way to go here. However, it might be worth adding that the use of wp_localize_script _is_ valid for actual localization strings etc as this isn't REST API / data related.

For the date / date_gmt issue / discussion - I opened #38883 to track that problem, however I don't think this issue affects this ticket anyway, as we want to show dates in the timezone of the admin, not the user who is viewing the admin. This means date is actually the field we want in this case. I recognise this might not be true for other decoupled implementations but I don't think it should hold up this task. We are trying to improve the API is many ways in other tickets, we don't need to make this the ticket to represent every use case for the API.

[adamsilverstein]

@joehoyle thanks for taking this up! I like the direction of your latest patch and will test it out... I would like to work towards getting actual gmt data in the database (and api response), thanks for opening #38883.

@aduth makes a good point that we still are losing potentially useful hooks. Long term, we need to think more here about forward compatibility than backward compatibility. That is if we deprecate a filter or hook developers use in PHP, can we provide a similar standard approach for developers to use in our new JavaScript application? Once we have standard JavaScript actions and filters (#21170) we can include matching/similar JavaScript filters for wp_trim_words, get_edit_post_link and even dashboard_recent_drafts_query_args.

@jnylen0 Thanks for the feedback, the use of wp_localize_script seems reasonable here since this client lives in the admin. I agree reading site settings just to figure out a proper date is not ideal.

[adamsilverstein]

I tested out 38342.8.diff​ and everything works as expected. I tried out my previous filter for dashboard_recent_drafts_query_args and that also worked fine. (I did catch one unrelated bug in the JS client I'm going to open a separate ticket for).

[adamsilverstein]

38342.9.diff​ corrects the issue @jnylen0 noted above

[pento]

Reading through 38342.9.diff​, here are some thoughts:

• wp_dashboard_recent_drafts() should be deprecated, and the docs for dashboard_recent_drafts_query_args moved to rest_filter_quick_draft_query().
• I'm not too worried about losing the wp_trim_words filter, but losing the get_edit_post_link filter will likely be be a problem.
• There's no fallback for the post title being empty, which _draft_or_post_title() used to take care of. This is necessary for the edit link to work.
• Are the 'object' === typeof attributes.content and 'object' === typeof attributes.title checks necessary? These attributes should always be objects.
• I'm not wild about the date formatting code. It feels messy to be doing it in the normalizeAttributes() method. I tend to agree that the API shouldn't return a formatted date, so a wp.formatting-based method for formatting a date would useful to introduce a standard practice. Using Intl with a fallback seems like a reasonable method, that could be upgraded at a later date. Integrating something like Moment.js, as @jnylen0 suggested, is a large task that could not be accomplished quickly.
• I agree with @adamsilverstein that wp_localize_script() is fine for Dashboard-based features.

Generally speaking, the JavaScript feel overly verbose, I found it tricky to keep track of code paths in my head, which shouldn't be happening for such a simple feature. Developer inaccessible JavaScript has been a major inhibitor of gaining new JavaScript core contributors in the past (see: the difficulties we have in maintaining the Media modal), I really don't want us to be encoding standard practices that make it harder for folks to get started with contributing.

[aduth]

Are the 'object' === typeof attributes.content and 'object' === typeof attributes.title checks necessary? These attributes should always be objects.

These are normalized because the collection manages both posts from the server (where content is an object) and posts newly created in the client prior to have being saved (where content is a string).

I'm not wild about the date formatting code.

With good utilities, I could see this logic living in the template. The bulk of the current logic is converting to the site time on the client. What do you envision being the arguments and return value of the utility method you mentioned? I'm not familiar with what's involved in adding a library like Moment.js, but given the quirks and variations in parsing and formatting dates, I couldn't recommend it enough.

Generally speaking, the JavaScript feel overly verbose

Are there specific parts that could be simplified? Looking over it again, my observations are:

• It would be nice if we could avoid normalizeAttributes
• Perhaps we could simplify the prompt (i.e. placeholder) functions (currently 5 functions related to it)
• Do we even need setActiveEditor? This was a holdover from previous logic
• The display of errors in the Form view is perhaps indirect, though I think fits well with the Backbone view paradigm

[joehoyle]

Per @pento's note on verbosity, I added a new patch removing the custom modal and collection for drafts - I instead just took the bit of formatting code and put it in the ListItemView, and moved the order_by to a REST API argument. I think this approach is nicer as it directly uses wp.api.models.Post and wp.api.collections.Posts which is a better showecase IMO.

[joehoyle]

Offload validation to the backend, handled i18n automatically

[joehoyle]

wp_dashboard_recent_drafts() should be deprecated, and the docs for dashboard_recent_drafts_query_args moved to rest_filter_quick_draft_query().

Added in 38342.12.diff - however I've not depracated a function before!

There's no fallback for the post title being empty,

Added in 38342.12.diff

Are the 'object' === typeof attributes.content and 'object' === typeof attributes.title checks necessary? These attributes should always be objects.

I changed this in 38342.12.diff to be a fair bit simpler. We just take the value in the render() method now - you are right, those fields always do exist.

Generally speaking, the JavaScript feel overly verbose, I found it tricky to keep track of code paths in my head, which shouldn't be happening for such a simple feature....

I tried to make things a bit simpler in the latest patch by not declaring our own models and collections, instead using using wp.api.collections.Posts etc which I think is better for example purposes too.

[ocean90]

38342.12.diff​

• <?php esc_html_e( 'Drafts' ) ?> and <?php esc_html_e( 'Loading…' ) ?> should be without esc_html(), the latter should use ….
• While we're here: To improve a11y we should probably make use of wp.a11y.speak().
• grunt precommit:js complains about a missing var declaration: 'date' is not defined.
• grunt precommit:css generates the missing vendor prefixes for the animation, make sure to run that before the commit.
• Can we have some QUnit tests for wp.formatting.trimWords()?
• The currently title is a number because of attributes.title.rendered.length.
• JS variables in script-loader.php should be camelCase, no_title => noTitle.

[joehoyle]

Added a new patch that lazy-loads QuickPress, so it's not initted / doesn't fetch data from the API if the Quick Draft is not checked in Screen Options.

[ocean90]

Replying to ocean90:

38342.12.diff​

• The View All links is displayed if the collection has more than one drafts, but the comment above this.$el.find( '.view-all' ).toggle( this.collection.length > 0 ); says "is visible if 5 or more drafts"
• The error notice (​https://cloudup.com/cX88Fx1mGrr) should use the alternative style (.notice-alt) and be above the form. Maybe we should prevent submitting an empty form?

[joehoyle]

In 14:

<?php esc_html_e( 'Drafts' ) ?> and <?php esc_html_e( 'Loading…' ) ?> should be without esc_html(), the latter should use &hellip;.

Fixed, my bad - I usually escape translations.

grunt precommit:js complains about a missing var declaration: 'date' is not defined.

Fixed up warnings here.

grunt precommit:css generates the missing vendor prefixes for the animation, make sure to run that before the commit.

I ran it in the patch too, to make sure.

The currently title is a number because of attributes.title.rendered.length.

Doh, fixed!

JS variables in script-loader.php should be camelCase, no_title => noTitle.

Fixed

Still need to do:

1. While we're here: To improve a11y we should probably make use of wp.a11y.speak().
2. Can we have some QUnit tests for wp.formatting.trimWords()?
3. The View All links is displayed if the collection has more than one drafts
4. The error notice (​https://cloudup.com/cX88Fx1mGrr) should use the alternative style

[adamsilverstein]

Replying to joehoyle:

Looks great, thanks! I will work on these final items.

Still need to do:

1. While we're here: To improve a11y we should probably make use of wp.a11y.speak().
2. Can we have some QUnit tests for wp.formatting.trimWords()?
3. The View All links is displayed if the collection has more than one drafts
4. The error notice (​https://cloudup.com/cX88Fx1mGrr) should use the alternative style

[adamsilverstein]

In 38342.15.diff​ :

• Added calls to wp.a11y.speak() for success and failure when adding a draft. (not sure why but I didn't hear failures in voiceover/chrome although the code executed).
• Added some QUnit tests for wp.formatting.trimWords(), based on out phpunit tests for wp_trim_words, I only left out the tag and style removal tests as those don't apply here.
• Clean up logic for showing View All link, using this.collection.hasMore()
• Switched the error notice to use the alternate style and put it above the form.

The error notice looks like this now:

I also cleaned up the inline docs a little and am going to continue working on that.

[pento]

Reviewing 38342.15.diff​:

• In wp_dashboard_print_recent_drafts_template(), the edit URL is still hard coded. This is a blocker, we cannot be setting the example of hardcoding URLs that previously could be filtered.
• The @deprecated comment should use the full version, 4.7.0.
• Is rest-api.php the best place for a filter that only needs to be run on the Dashboard home?
• None of the QuickPress functions have JS docs. Let's set a good example. :-)
• Having the timezone offset (and the current user ID, for that matter) come from the Quick Draft settings is not very extendable. This is information that should be available to all features as they're ported, rather than each one needing to reimplement it.

Replying to aduth:

I'm not wild about the date formatting code.

With good utilities, I could see this logic living in the template. The bulk of the current logic is converting to the site time on the client. What do you envision being the arguments and return value of the utility method you mentioned? I'm not familiar with what's involved in adding a library like Moment.js, but given the quirks and variations in parsing and formatting dates, I couldn't recommend it enough.

A good start would be a function that does exactly what the current formatting code is doing - take a date (ISO8601, Date object or unix timestamp), and return it with the site's current date formatting settings.

Generally speaking, the JavaScript feel overly verbose

Are there specific parts that could be simplified?

As @joehoyle mentioned elsewhere, this was a bit a rabbit hole to poke at, without really going into detail. Let me try and expand a bit. :-)

I'm looking at this patch with the firm point of view of "how does this look as a pattern for existing Dashboard features to be ported to the REST API in the future?". With that in mind, I keep running into problems. For such a small feature, we've had to make quite a few architectural compromises - some of these have been fixed, some of them seem to be genuine sticking points, or symptoms of larger problems. I'm not wild about the idea of continually running into these same problems every time we try to port a feature across.

When I read the QuickPress code, it's not dissimilar in structure to the media grid, with which we've significant problems maintaining over the years. Is that a problem with all Backbone-based code? Would we face similar problems if we were to use other libraries?

The decision to use Backbone seems to be based on the fact that we always use Backbone. Given that we're talking about the future of WordPress Dashboard development, I really would like to see some exploration of other technologies. React has gone really well for Automattic with Calypso. Vue.js seems to be gaining a lot of traction with folks who find React to be overkill. There hasn't been much discussion on what the best direction here might be.

At the end of the day, I don't think that keeping it as Backbone is a blocker. But the original point of this ticket was to both test for shortcoming in the endpoints, which it has done successfully; and spark investigation into what the future of WordPress Dashboard development will look like, which it hasn't.

[netweb]

Patch 38342.16.diff​ updates 38342.15.diff​ per WordPress CSS Coding Standards (No other changes)

[adamsilverstein]

Replying to pento:

Reviewing 38342.15.diff​:

Thanks for the additional feedback @pento! I will try to address your concerns in an upcoming patch.

Regarding the choice of Backbone here - I'm open to exploring other technologies such as Vue or React, however I have a feeling Backbone is the right tool for this task. Using Backbone makes sense here because buys us a lot of built in functionality: we leverage syncing and the Backbone JS client, and leverage collections to handle management of the draft list. I find the backbone code here straightforward to understand and follow, this is a much simpler feature than the media library. What advantages might we hope to find by switching to React or Vue?

[adamsilverstein]

38342.17.diff​

• First pass at adding additional inline docs
• remove setActiveEditor handlers, these appear unused and extraneous unless I am missing something

working on the other issues next

[afercia]

Hello everyone :)

Replying to adamsilverstein:

• Added calls to wp.a11y.speak() for success and failure when adding a draft. (not sure why but I didn't hear failures in voiceover/chrome although the code executed).

That's probably because of this bug: #36853

Also, I'd recommend to use the assertive parameter when calling wp.a11y.speak(), this way the success/error messages will be announced immediately, without waiting for the screen reader to finish what it's currently reading out.

I'd suggest to review a bit the error messages too. Currently, the visible ones are based on a try/catch and when there is a response that returns an error the message is:
1 - "Content, title, and excerpt are empty."
when there is no response:
2 - "An error has occurred. Please reload the page and try again."

Instead, the audible error message is always the same:
3 - "An error has occurred. Please reload the page and try again."

About 1, there's no "excerpt" field here. I see the API uses wp_insert_post() but the message is not appropriate in this case.

About 3, not sure why it should be different from the visual one. I'd say it should be paired with the visual messages to distinguish the two cases.

Other quirks noticed about error handling:

1
after a draft is created successfully:
leave empty the form and submit again
no error message is displayed

2
refresh the page
leave empty the form and submit again
the error message is correctly displayed
now fill the form fields and submit
the new draft is correctly created but the error message is still displayed

3
the error notice is displayed with the wrong colors, it currently uses a wrong CSS class notice-info should be notice-error instead

Lastly, I'd consider to avoid inline styles e.g. style="display: none;", and use the CSS class hidden when possible.

I'm also noticing a couple more things that should be probably improved about date formatting and doubled entries, will try to post later after some more testing.

[afercia]

Couple more issues.

1
I'm not sure the locale in Intl.DateTimeFormat can be left to undefined. I live in Italy, but I keep my operating system and browsers in English. Seems there are inconsistencies across browsers, see the screenshot below.

Notice the OS default language is set to a "generic" English (no en-GB or en-US) and Chrome on the left displays an en-GB date format while Firefox on the right an en-US one. I have to manually change my OS language to en-US to get the same behaviour in both browsers. Besides the inconsistency, seems setting the locale to undefined makes the date format dependent on the OS language, at least on macOS. This may vary on other platforms.

By the way, the main issue is that I've set the admin UI in French :) Regardless of the OS or browser language, the date should be displayed in the format of the language set in the user profile.

2
When quickly clicking twice the "Save Draft" button, I get two new drafts. While the list below the form shows just one new draft, on the Posts > Drafts screen I have two new ones. This is probably because the button doesn't get disabled when submitting the form as in the previous AJAX implementation.
Note: when the form gets re-rendered, focus should be moved to the title field.

[adamsilverstein]

@afercia Thank you for the testing, detailed feedback and suggestions!

Regarding using assertive I was uncertain if that was appropriate for both errors and success messages, or is it only for failures (ie when should we tell the screen reader to be assertive)?

Regarding adding locale to Intl.DateTimeFormat, that indeed looks like an issue, I think we can localize/pass the correct locale string for this function to resolve this.

Thanks again for the extensive feedback here, I will work to address the points you raised.

[afercia]

@adamsilverstein thanks :)

Regarding using assertive I was uncertain if that was appropriate for both errors and success messages, or is it only for failures (ie when should we tell the screen reader to be assertive)?

I'd say polite/assertive are not specifically tied to semantics (normal messages vs errors/warnings). It's just about timings. If the screen reader is already reading out something, e.g. a long sentence, polite won't interrupt it. Instead, assertive interrupts immediately. Should be used with care to don't stop users from completing their current task. In this case, for both confirmation and error, the task is completed so my thought was to give users an immediate feedback.

Also, the politeness levels are just "suggestions" and screen readers could theoretically choose to ignore them. Quoting the specification:

Politeness levels are essentially an ordering mechanism for updates and serve as a strong suggestion to user agents or assistive technologies. The value may be overridden by user agents, assistive technologies, or the user. For example, if assistive technologies can determine that a change occurred in response to a key press or a mouse click, the assistive technologies may present that change immediately even if the value of the aria-live attribute states otherwise.

[adamsilverstein]

38342.19.diff​

• Dry out error handling with ‘setErrorState’
• Introduce an app state model for state tracking the app's state
• Rename root objects QuickPress -> QuickDraft

[adamsilverstein]

38342.20.diff​

• Add a save lock on the state model that prevents the double save noted by @afercia
• Switch from inline display:none to toggling the 'hidden' class
• Use notice-error instead of notice-info class for error display

[adamsilverstein]

@afercia I believe I have addressed all of your feedback other than the Date issues you noted. When you have a chance, can you please retest and see if I missed anything? Thanks!

[adamsilverstein]

38342.21.diff​ addresses some feedback from https://core.trac.wordpress.org/ticket/38342#comment:69:

• Use ‘admin_url’ to generate the admin url link in the template (so its filterable)
• Update docs, @since 4.8.0, fix deprecated notice use full version

[adamsilverstein]

38342.22.diff​

• move draft template below form
• docs cleanup
• consolidate logic for hasValuesToSave
• include missing unit tests
• fixes for jshint

[aduth]

Latest diff includes:

• Unit tests for wp.formatting.trimWords (​4681d3f on GitHub)
• New wp.formatting.date date formatting utility (​4314aec on GitHub)

The idea with the date formatting utility is to try to mimic the behavior of ​PHP `date`. This was sparked by @pento 's suggestion at ticket:38342#comment:69 . It succeeds at this to some degree, but is really only a rough approximation. This becomes obvious when either trying to customize the separators (/ vs. -) or when the site is configured to show day-first dates in an en-US locale (where Intl will format month-first). Intl.DateTimeFormat doesn't give much control over this, except perhaps through its ​`formatToParts` method which has abysmal browser support.

As implemented, it should solve two present issues:

• The date respects the current user locale (via 4.7.0's ​get_user_locale)
• (Tries to) format date according to site-configured setting, at least working between numeric/written month

I had considered also some attempts to port date directly to JavaScript, like ​that of the Locutus project. The downside here is trying to handle localization of month and weekday names.

Considering Moment.js again, there are similar concerns with localization (​though includes proper support), and also incompatibilities between PHP date formats (which could be ​resolved with mappings).

I haven't yet written tests for wp.formatting.date since I wasn't sure yet whether this is a direction we'd want to take.

[afercia]

Re: date formatting, see also #39222.

@adamsilverstein sorry for the late reply and thanks for all your work here! Tested 38342.22.diff​ so not looking into the Date issues and it looks good to me, except one thing I haven't considered before.

The thing is, even if the speak messages are now assertive, after 250ms focus is moved to the Title input field so screen readers will stop announcing the message and will start announcing the input field. Tested with Safari+VoiceOver and Firefox+NVDA what they announce is something like:
An err... Title edit text
or:
Suc... Title edit text
which defeats the purpose of sending audible messages altogether. I've tried to delay the messages too but it doesn't work well. I'd consider to don't move focus at all (note: it doesn't happen when ! hasValuesToSave) and keep it on the button. I understand moving focus has some benefit for users who are going to post multiple drafts but in this case it breaks accessibility and haven't found other ways to make it work.

[aduth]

Following-up on my previous comment at ticket:38342#comment:84, there are a few revisions that need to be made to the date formatting, particularly because it's hard to know (a) the true timezone offset of the incoming date parameter and (b) whether the return value should always be in the site timezone, browser timezone, or even ad hoc offsets.

In 38342.23.diff​, the value passed from dashboard.js is a date string in the site's timezone (post.date). Creating a Date object from the string automatically adjusts it to browser's local timezone. This is why the formatting function "adjusts back to GMT+0", but only works because what we think is GMT+0 is actually the site's timezone, even when it might be in-fact offset from UTC.

Based on the incoming date values, we could make the following assumptions:

• A numeric value is GMT+0 (timestamp, number of milliseconds since 1970-01-01 00:00:00 UTC)
• undefined is GMT+0 since it defaults to Date.now and is treated the same as timestamp
• A string, if ISO8601, could include offset details. Dates from the REST API do not include these offset details. ​Per spec, I would expect they'd be treated as local browser time, but in my testing Chrome interprets them as GMT+0 (not sure if this is a browser-specific implementation detail, try new Date( '2017-01-01T00:00:00' ) in your browser console)
• A date object cannot be known with certainty, since it depends on how it was created. The Date instance itself will be in the browser's timezone, but if it was created from an API post object's date property (as in our Quick Drafts implementation), it would be adjusted by the site's timezone.

So it would seem we either need conventions around how to call the function (must already be in a predictable offset) or add hinting options to allow specifying the browser, site, or ad hoc timezone offsets.

Perhaps something like:

wp.formatting.date( wp.api.utils.parseISO8601( attributes.date ), 'F j, Y', { from: 'site', to: 'site' } );

Or even a fluent jQuery-like API (noting that default format should respect site option):

wp.formatting.date( new Date() ).from( 'browser' ).to( 'site' );

For reference, equivalent in moment-timezone looks something like:

moment.tz( '2013-11-18 11:55', 'America/Toronto' ).tz( 'America/Los_Angeles' ).format();

[jnylen0]

Replying to aduth:

there are a few revisions that need to be made to the date formatting, particularly because it's hard to know (a) the true timezone offset of the incoming date parameter

Tracking this enhancement at #39854.

and (b) whether the return value should always be in the site timezone, browser timezone, or even ad hoc offsets.

Timezones haven't been satisfactorily handled in the API in general. A draft's date_gmt can be zero/null (#38883). There are major issues with timezone conversion when creating and updating objects (#39256). Finally, I think we should add the timezone to all date responses in the REST API to provide some more clarity here (#39855).

[adamsilverstein]

In 38342.6.diff​

• Include wp-hooks library from #21170
• Remove PHP side back compat hook handling for dashboard_recent_drafts_query_args
• Add new JavaScript filter for dashboard_recent_drafts_fetch_args
• Don't refocus title after new draft creation, as per @afercia's comment

With this change, instead of supporting the previous PHP filter, we provide a new way to filter the query used for the list view. Developers who previously filtered the query used for the post list with dashboard_recent_drafts_query_args must now use the JavaScript filter dashboard_recent_drafts_fetch_args to achieve the same results.

For the earlier example PHP filter:

add_filter( 
	'dashboard_recent_drafts_query_args', 
	function( $query_args ) {
		$query_args['posts_per_page'] = 10;
		return $query_args;
	} 
);

Becomes this in JavaScript:

wp.hooks.addFilter( 
	'dashboard_recent_drafts_fetch_args', 
	function( $args ) {
		$args['per_page'] = 10;
		return $args;
	} 
);

I itried to keep the filter naming similar, looking at it now though I'm tempted to change the filter to camel case as in dashboardRecentDraftsFetchArgs which also makes it clearer this is a JS filter by the name alone.

[kadamwhite]

After discussion in the #core-restapi weekly chat today with @adamsilverstein, I'm closing this as wontfix. It's a good plan, and was a reasonable thing to pursue post-merge, but at this stage any overhaul to this functionality feels like it should fall under the overall Gutenberg admin changes as we move into full-site editing.