Add wporg/time block #414

adamwoodnz · 2023-07-11T01:05:11Z

Closes #413

This PR reimplements the existing [time] shortcode as a Gutenberg format.

The block uses the Format API so that the time can be applied to a string within a parent block, eg. a paragraph.

Notable changes from the existing shortcode implementation:

The reformatting is achieved by filtering and modifying the_content rather than the shortcode content
The time parsing (parse_time) is separated from the dom manipulation (transform_time_blocks)
The time parsing is always relative to the post date, there is no attribute support
The frontend time_converter_script no longer has a jQuery dependency

Screenshots

Editor	Frontend

Testing

Create a post, add various valid (eg. 'Tuesday, April 5th, at 15:00 UTC') and invalid datetime descriptions, and format as a Time using the option in the rich text toolbar
Save and view on the frontend
The date displayed should be reformatted to your timezone
Test various styles
Change the post date and check that the displayed time changes too
Add multiple Times and check that the JS is only added to the page once
Test cross browser and include Safari

mu-plugins/blocks/time/index.php

ryelle

This is registering a block on the server, but the code in index.js doesn't actually create a block. While register_block_type works, it would be more correct to register the script a different way. You could try the method I used for showcase, which still uses the block.json and the generated index.asset.php.

Overall, the references to this being a block are not accurate - the parent folder & block.json are fine because those are necessary for the build process (much like the "style" block we use in the themes). For clarity though it would be best to update those references (like in the docs).

It looks like we never set up php linting on this project, but I see a few issues intime/index.php. You can run composer run lint mu-plugins/blocks/time to see them.

ryelle · 2023-07-11T23:23:56Z

mu-plugins/blocks/time/index.php

+ */
+function time_converter_script() {
+	?>
+	<script type="text/javascript" id="time_converter_script">


Could you add this on the frontend via a viewScript?

I did try that with no success, I'll have another go

Added a viewScript entry but it isn't automatically enqueued. I've used something similar to the method you used for showcase for this also

ryelle · 2023-07-11T23:25:03Z

mu-plugins/blocks/time/src/index.js

+	tagName: 'a',
+	className: 'wporg-time',


This creates a link that doesn't do anything — could you maybe use the time element instead? Use the PHP to add the datetime attribute, and then use that to build up the element in JS?

ryelle · 2023-07-11T23:30:47Z

mu-plugins/blocks/time/index.php

+		$new_time_content = $dom->createElement( 'abbr', $time_content );
+		$new_time_content->setAttribute( 'class', 'wporg-time-date' );
+		$new_time_content->setAttribute( 'title', gmdate( 'c', $parsed_time ) );


Do you know if there has been any feedback on the accessibility of this abbr + title approach?

I don't, I'll ask

I asked, feedback was that a time element would be better, and in particular the title on the abbr could be bad.

ryelle · 2023-07-11T23:35:39Z

The time parsing is always relative to the post date, there is no attribute support

A quick search on make.w.org/core shows that they use the relative attribute often, so it would be good to think about how we could add support (maybe in the future).

adamwoodnz · 2023-07-17T04:01:07Z

mu-plugins/blocks/time/index.php

+
+	$metadata = get_metadata();
+	$time_converter_script_handle = register_block_script_handle( $metadata, 'viewScript', 0 );
+	wp_enqueue_script( $time_converter_script_handle, null, null, filemtime( __DIR__ . '/build/convert_times.js' ), true );


I've added all the parameters so that it loads in the footer. Not sure of there is a better way.

adamwoodnz · 2023-07-17T04:02:16Z

mu-plugins/blocks/time/index.php

+	}
+
+	$metadata = get_metadata();
+	$time_converter_script_handle = register_block_script_handle( $metadata, 'viewScript', 0 );


Couldn't find a nice way to register this script in init for better performance, and then only enqueue it here

adamwoodnz · 2023-07-17T04:04:09Z

It looks like we never set up php linting on this project, but I see a few issues intime/index.php

After my MacOS upgrade to Ventura my php version went back to 8 and I had phpcs errors which blocked me seeing these in VSCode. Fixed now.

adamwoodnz · 2023-07-17T04:11:16Z

mu-plugins/blocks/time/index.php

+	$dom = new \DOMDocument();
+
+	// Ignore warnings about htlm5 tags.
+	$dom->loadHTML( $content, LIBXML_NOERROR );


Without this there can be many warnings displayed (nav, time, and many other html5 tags aren't supported).

Alternatively we could use regexp or a different dom parser...

mu-plugins/blocks/time/index.php

adamwoodnz · 2023-07-17T22:41:09Z

Feedback received elsewhere: it would be best to avoid filtering the_content on page view, especially if it involves loading a DOM parser.

Some possible solutions for this:

hook into the content save flow for php based DOM manipulation (probably a filter)
use php regexp to find and replace instead of DOM parsing
do all the parsing and DOM manipulation client side

…g scripts

…enqueuing scripts" This reverts commit a5244fd.

adamwoodnz · 2023-07-18T04:06:31Z

Alternative solution using client side parsing in #415

adamwoodnz · 2023-07-25T22:55:37Z

Closing in favour of #415

adamwoodnz linked an issue Jul 11, 2023 that may be closed by this pull request

Add support for [time] shortcode #413

Closed

adamwoodnz self-assigned this Jul 11, 2023

adamwoodnz marked this pull request as draft July 11, 2023 01:07

adamwoodnz force-pushed the try/413-add-support-for-time-shortcode branch from 4cf22a5 to 357ef4e Compare July 11, 2023 01:13

adamwoodnz commented Jul 11, 2023

View reviewed changes

mu-plugins/blocks/time/index.php Outdated Show resolved Hide resolved

adamwoodnz force-pushed the try/413-add-support-for-time-shortcode branch from 357ef4e to e6b167e Compare July 11, 2023 05:20

Add wporg/time block

93160a3

adamwoodnz force-pushed the try/413-add-support-for-time-shortcode branch from e6b167e to 93160a3 Compare July 11, 2023 05:23

adamwoodnz requested a review from ryelle July 11, 2023 05:48

adamwoodnz marked this pull request as ready for review July 11, 2023 05:48

Remove debug

ff37454

adamwoodnz requested review from dd32, StevenDufresne and tellyworth July 11, 2023 05:49

Stop parsing content if empty

be048ab

ryelle requested changes Jul 11, 2023

View reviewed changes

adamwoodnz added 3 commits July 17, 2023 11:53

Remove usage of register_block_type as this isn't a block

8c87c3f

Fix phpcs errors

f8058d6

Enqueue frontend script if required rather than rendering inline

f82d9c6

adamwoodnz force-pushed the try/413-add-support-for-time-shortcode branch from 7e89b0d to 75d1963 Compare July 17, 2023 03:55

adamwoodnz commented Jul 17, 2023

View reviewed changes

pkevan reviewed Jul 17, 2023

View reviewed changes

mu-plugins/blocks/time/index.php Outdated Show resolved Hide resolved

adamwoodnz added 3 commits July 18, 2023 12:18

Change to semantic elements for time on frontend

e6eb98b

Fix transform_times bail out logic

c3f936f

Use Gutenberg non block example approach for registering and enqueuin…

018f389

…g scripts

adamwoodnz added 3 commits July 18, 2023 12:18

Remove unused helper

0dd43dc

Fix bail out logic for no time elements

8eb9242

Revert "Use Gutenberg non block example approach for registering and …

11329c5

…enqueuing scripts" This reverts commit a5244fd.

adamwoodnz force-pushed the try/413-add-support-for-time-shortcode branch from 90b0196 to 11329c5 Compare July 18, 2023 00:19

adamwoodnz mentioned this pull request Jul 18, 2023

Add time format with client side date parsing #415

Merged

adamwoodnz added the [Block] New Block label Jul 21, 2023

adamwoodnz closed this Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add wporg/time block #414

Add wporg/time block #414

adamwoodnz commented Jul 11, 2023 •

edited

Loading

ryelle left a comment

ryelle Jul 11, 2023

adamwoodnz Jul 16, 2023

adamwoodnz Jul 17, 2023 •

edited

Loading

ryelle Jul 11, 2023

adamwoodnz Jul 17, 2023

ryelle Jul 11, 2023

adamwoodnz Jul 16, 2023

adamwoodnz Jul 25, 2023

ryelle commented Jul 11, 2023

adamwoodnz Jul 17, 2023

adamwoodnz Jul 17, 2023

adamwoodnz commented Jul 17, 2023

adamwoodnz Jul 17, 2023

adamwoodnz commented Jul 17, 2023

adamwoodnz commented Jul 18, 2023

adamwoodnz commented Jul 25, 2023

Add wporg/time block #414

Add wporg/time block #414

Conversation

adamwoodnz commented Jul 11, 2023 • edited Loading

Notable changes from the existing shortcode implementation:

Screenshots

Testing

ryelle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamwoodnz Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryelle commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamwoodnz commented Jul 17, 2023

Choose a reason for hiding this comment

adamwoodnz commented Jul 17, 2023

adamwoodnz commented Jul 18, 2023

adamwoodnz commented Jul 25, 2023

adamwoodnz commented Jul 11, 2023 •

edited

Loading

adamwoodnz Jul 17, 2023 •

edited

Loading