Skip to content
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 Archives block #5495

Closed
wants to merge 66 commits into from
Closed

Add Archives block #5495

wants to merge 66 commits into from

Conversation

miina
Copy link
Contributor

@miina miina commented Mar 8, 2018

Uses #5602

Continued archives block implementation from #1518. Archive block implementation that uses similar output and filters as the archives widget. Has alignment toolbar within the block (left, center, right).

Screenshots:

Block toolbar:
screen shot 2018-05-01 at 2 23 23 pm
Inspector toolbar:
screen shot 2018-05-01 at 2 17 54 pm
Dropdown view:
archives-block-dropdown
List view:
archives-block-list

Fixes #1464.

@miina
Copy link
Contributor Author

miina commented Mar 8, 2018

Specs for the possible archives REST API endpoint in case of going that direction:

  • Read only (wp/v2/archives);
  • Returns list of archives, for example:
{
	0: {
		'text': 'February 2018',
		'href': 'http://gutenberg.local/2018/02/',
		'html': '<li><a href="http://gutenberg.local/2018/02/">February 2018</a>&nbsp;(11)</li>',
		'count': 11,
	},
	1: {
		...
	}
}
  • Accepts the following params (same as wp_get_archives() except for echo):
    -- type ('daily', 'monthly', etc.);
    -- limit;
    -- format;
    -- before;
    -- after;
    -- show_post_count;
    -- order;
    -- post_type;

Thoughts?

@westonruter
Copy link
Member

I don't think that the response should include html. Also I don't think parameters for format, before, or after should be included. The endpoint who's return the raw data and the client can format it as desired.

@miina
Copy link
Contributor Author

miina commented Mar 8, 2018

Okay. I was thinking that maybe including html could make it more straightforward to match the result of wp_get_archives(), and also thinking of some other elements (e.g. post title / attachment caption that have a raw version + html version) so the API user can choose.

However, I guess from API's perspective it makes more sense to just return raw data in this case 👍

@afercia afercia mentioned this pull request Mar 9, 2018
@Lewiscowles1986
Copy link
Contributor

Lewiscowles1986 commented Mar 12, 2018

Have to agree with @miina approach. Other works on visual shortcodes are returning HTML from WP, including CSS + JS

@Lewiscowles1986
Copy link
Contributor

Lewiscowles1986 commented Mar 12, 2018

The main problem with this for me is what to do about content that is added with the archives block. Is the content only for when there is nothing in the archive, is there an option for that. Will it fall-back to the built-in WP content experience (with oEmbed & shortcode) etc?

If it does, I think running $output_content = apply_filters('the_content', $content); might be an option, but it goes against the block based nature of Gutenberg (where things are separate and re-orderable). You could, for example, encourage users to use the paragraph block if they desire that experience, then focus solely on the archives (perhaps a custom JS + CSS + raw HTML as part of config).

@miina miina mentioned this pull request Mar 14, 2018
3 tasks
westonruter and others added 4 commits March 14, 2018 11:26
…t of the REST API

* Singularize "blocks renderer" to "block renderer"
* Rename 'output' property to 'rendered' property.
* Re-use get_item_permissions_check and get_item method names.
* Supply block attributes schema as endpoint schema.
* Introduce attributes endpoint property and let REST API schema validate and sanitize them.
* Ensure that attribute values are sanitized in addition to validated.
Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern at this point is that, by relying on the front-end rendering output, we end up with some strange situations. The largest is the handling of links:

gut

while another issue, as mentioned inline, lies in the existence of repeated HTML element IDs.

*/
function render_block_core_archives( $attributes ) {
static $block_id = 0;
$block_id++;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way this is used in tandem with ServerSideRender, this will generally lead to ID conflicts. From my own local testing:

document.querySelectorAll( '#wp-block-archives-1' )

returns multiple elements if I have more than one Archive block in my editor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting that implementing an edit for this block type would allow the editor to retain more control over this kind of information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added using uniqid instead, thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcsf Also disabled the pointer-events of the links, this way it'll not cause the leaving page alert, however, the links are not clickable within the editor. Probably better but not ideal, thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added using uniqid

That should do for now, thanks.

disabled the pointer-events of the links

It doesn't solve for other interactions (e.g. when attribute Display as dropdown is on). It's indeed not ideal, but I don't have a good solution. I mean, sure, we could also disable pointer-events on select elements, but then even more interaction is lost, perhaps leading the user to perceive the experience as "buggy" and, thus, confusing. On the other hand, hijacking select elements to remove their onchange attribute could technically help, but that sounds like a very bad precedent to set. /cc @mtias

static $block_id = 0;
$block_id++;

$show_post_count = ! empty( $attributes['showPostCounts'] ) ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ternary unnecessary

// for any boolean A,
( A ? true : false ) === A

onChange={ ( nextAlign ) => {
setAttributes( { align: nextAlign } );
} }
controls={ [ 'left', 'center', 'right', 'full' ] }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why full is supported but not wide. (If both should be, the controls prop doesn't need to be passed at all.)

Copy link
Contributor Author

@miina miina Jun 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it should support full either, removed it.

Edit: Originally based this on other dynamic blocks, e.g. Categories and Latest Posts. Not sure if there's a standard for which controls to support. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not aware of a proper standard. I think it's fine to remove both for now.

In any case, cc'ing @jasmussen to make sure you're aware of these differences :)

</Fragment>
);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the life cycle of the ArchivesBlock component, and the fact that the block proper isn't very interactive, I'm not sure we need a class-based component and the ahead-of-time binding of the toggle* methods. In other words, edit could just be a functional component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcsf Curious if that's a matter of preference or would there be benefits for using edit as functional component instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's sort of been a tendency throughout Gutenberg to avoid what is, in a way, a performance trade-off that prioritizes performance of repeated rendering, at the (slight) cost of init-time bindings and increased memory footprint (which may add up with every little component) and at the (arguably) higher cost of code complexity for newcomers. For a component like ArchivesBlock, I'm tempted to say we're better off going with something simpler (i.e. functional component) even if it means binding two event handlers when rendering.

More on the subject of the cost of render-time inline functions: https://cdb.reacttraining.com/react-inline-functions-and-performance-bdff784f5578

'type' => 'boolean',
'default' => false,
),
'align' => array(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor/subjective, but the whitespace resulting from keeping certain elements aligned across lines can be distracting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter enforces this, for better or worse 🙂

@youknowriad youknowriad modified the milestones: 3.0, 3.1 May 29, 2018
@gziolo gziolo modified the milestones: 3.1, 3.2 Jun 19, 2018
@gziolo
Copy link
Member

gziolo commented Jun 19, 2018

@miina, moving to the 3.2 milestone. Do you need any help with this one, it wasn't updated in the last 18 days.

@miina
Copy link
Contributor Author

miina commented Jun 19, 2018

@gziolo Thanks for the update, latest change was updating the PR according to the review from @mcsf, haven't received a response to that yet, waiting for additional feedback.

@gziolo
Copy link
Member

gziolo commented Jun 19, 2018

Cool, so we will simply move it back to 3.1 if @mcsf is happy about the current shape of PR :)

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience, @miina.

Codewise I think we're essentially there, but I'm pretty concerned about the handling of interactive elements, namely those triggering navigation, as pointed out in inline comments.

To go into a broader discussion, I think this hurdle illustrates how tricky it can be to bridge server-rendered widgets (made for the specific context of front-end visualization; usually dependent on server logic but then also occasionally interacting with the client) and editor-facing blocks (highly interactive but also specifically built for the editing context). Solving for Archives will help for the next widgets that should be ported using <ServerSideRender />, but I suspect more challenges will be met.

Let me reflect a bit, and also see if someone else has a different perspective on this that helps us move forward.

@miina
Copy link
Contributor Author

miina commented Jul 5, 2018

@mcsf Any thoughts on the implementation so far?

For information that I'm going to be mostly offline now until the end of July -- if any changes would be necessary meanwhile to have the PR merged, anyone is welcome to pick it up.

@gziolo gziolo modified the milestones: 3.2, 3.3 Jul 5, 2018
@noisysocks
Copy link
Member

noisysocks commented Jul 6, 2018

To go into a broader discussion, I think this hurdle illustrates how tricky it can be to bridge server-rendered widgets (made for the specific context of front-end visualization; usually dependent on server logic but then also occasionally interacting with the client) and editor-facing blocks (highly interactive but also specifically built for the editing context).

Some similar reflections came up while adding the Playlist block (#7126). I think that, in general, we should avoid using <ServerSideRender> to implement blocks because it discourages direct manipulation which is one of our foundational design principles. For example, a block implemented using <ServerSideRender> would nearly always have to have its attributes adjusted using checkbox and dropdown controls that are in the sidebar.

Having said that, I think <ServerSideRender> is a totally acceptable approach for interim/iterative solutions (as in #7126) and in cases like this where the block is largely non-interactive. I'm happy with how this PR is implemented so long as we fix the issue you identified with links causing navigation.

That's my 2¢! Curious to hear what the leads think @mtias @karmatosed.

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code's looking really great! If we can think of a more durable solution than pointer-events: none, then I'm happy.

@@ -0,0 +1,4 @@
.gutenberg .wp-block-archives a {
pointer-events: none;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this doesn't completely disable the link. A user is still able to "click" it by focusing it using their keyboard and then pressing ENTER.

I'd try wrapping the <ServerSideRender> component with the <Disabled> component that we have and seeing what happens.

https://github.com/WordPress/gutenberg/tree/master/components/disabled

<InspectorControls key="inspector">
<PanelBody title={ __( 'Archives Settings' ) }>
<ToggleControl
label={ __( 'Show post counts' ) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably should make this title case for consistency with other inspector control labels in Gutenberg.

label={ __( 'Show Post Counts' ) }

onChange={ this.toggleShowPostCounts }
/>
<ToggleControl
label={ __( 'Display as dropdown' ) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same again here.

label={ __( 'Display as Dropdown' ) }

controls={ [ 'left', 'center', 'right' ] }
/>
</BlockControls>
<ServerSideRender key="archives" block="core/archives" attributes={ this.props.attributes } />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could replace this.props.attributes with attributes.


save() {
// Handled by PHP.
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there something we can return here as a fallback for clients like RSS readers that don't support dynamic blocks?

(Probably not, just wondering.)


category: 'widgets',

keywords: [ __( 'archives' ) ],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could remove this line. Gutenberg will treat the title (Archives) as an implicit search keyword.

'type' => 'boolean',
'default' => false,
),
'align' => array(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter enforces this, for better or worse 🙂

@talldan talldan mentioned this pull request Jul 13, 2018
@talldan
Copy link
Contributor

talldan commented Jul 13, 2018

Will continue implementation on #7949

@talldan talldan closed this Jul 13, 2018
talldan added a commit that referenced this pull request Jul 16, 2018
Co-authored-by: Pascal Birchler <[email protected]>
Co-authored-by: Miina Sikk <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
mcsf pushed a commit that referenced this pull request Jul 16, 2018
* Changes from PR #5495 squashed into single commit

Co-authored-by: Pascal Birchler <[email protected]>
Co-authored-by: Miina Sikk <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>

* Remove archived keywords, since the title is implicitly a keyword already

* Use already destructured property `attributes` over `this.props.attributes`

* Use title case for copy

* Use `Disabled` element to prevent interaction with Archives Block editor view

Use of `pointer-events` to disable interaction was problematic. It
didn't disabled the dropdown version of the block and it doesn't prevent
keyboard interaction.

The Disabled component handles these situations.

* Rename block.js to edit.js for consistency with other blocks

* Convert ArchivesEdit to a function component

* Remove `isSelected` boolean and inline InspectorControls

* Render archives list container as `ul` when a list and `div` when a dropdown

* Remove unnecessary React keys

* Improve description copy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Block Suggestion for a new block
Projects
None yet
Development

Successfully merging this pull request may close these issues.