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 "More" block. #1440

Merged
merged 24 commits into from
Jul 27, 2017
Merged

Add "More" block. #1440

merged 24 commits into from
Jul 27, 2017

Conversation

mtias
Copy link
Member

@mtias mtias commented Jun 26, 2017

image

See #983.

@mtias mtias added [Feature] Blocks Overall functionality of blocks [Type] Task Issues or PRs that have been broken down into an individual action to take labels Jun 26, 2017
@jasmussen
Copy link
Contributor

So this is interesting... the "READ MORE" text is editable.

Was this intentional on your part? I'm leaning towards this text not being editable, and letting themes decide the text that goes there. But it's a very intriguing idea.

@jasmussen
Copy link
Contributor

I dig the help text in the inspector! ⭐️⭐️⭐️⭐️⭐️

@mtias
Copy link
Member Author

mtias commented Jun 26, 2017

@jasmussen it is editable on purpose. WordPress handles it like this:
<!--more Your custom more text... -->

@jasmussen
Copy link
Contributor

Interesting! I did not know that.

Then I'd like to redesign it a bit. I'll assign myself this branch and do it in code, if that's okay with you.

@jasmussen jasmussen self-assigned this Jun 26, 2017
@mtias
Copy link
Member Author

mtias commented Jun 26, 2017

Sure thing. My plan was that the default text is not a placeholder but real text. Not sure it's trivial to get the text a theme has specified, so we may need to present it more abstractly.

@jasmussen
Copy link
Contributor

Not sure it's trivial to get the text a theme has specified, so we may need to present it more abstractly.

I think this is key to figure out if we actually want to make it editable.

What exactly does the "Your custom more text..." do in <!--more Your custom more text... -->? Does it override the Read More text provided by the theme? And if yes, is this a feature that we think will stick around for a bit? It sort of has that "abandoned" smell on it.

In case it's a feature we think will stick around, and it already works today, then I totally love the idea that not only can you customize it but do it on a per-post basis.

The key thing to think about for the design (you touch on it with the "get the text a theme has specified"), is making the configurability discoverable. If we can't actually read the text that the theme has provided, then we should either go super abstract and perhaps not even show the "Read More" text on the block, but something else entirely. Or maybe we do show that, but only let you customize the read more text in the inspector. Something I think I'll need to sleep on.

@mtias
Copy link
Member Author

mtias commented Jun 26, 2017

Yes, it works, but is quite the hidden feature:

image

I actually love that we could surface this way better with blocks, and that it's much more obvious how it works when you can edit the text directly.

@dmsnell
Copy link
Member

dmsnell commented Jun 26, 2017

I'm proposing that we leave this as its own block for now and iterate if necessary later. Essentially <!--something--> becomes shorthand for a block comment header. We will need to whitelist the available ones so we don't accidentally capture HTML comments.

// <!-- more -->

{
  blockName: 'wp:core/more'
}

// <!-- more read on! -->

{
  blockName: 'wp:core/more',
  attrs: {
    customText: 'read on!'
  }
}

@swissspidy
Copy link
Member

Can we somehow enforce that this block only can be used once in a post? Something for a separate PR, but just thinking out loud here.

@mtias
Copy link
Member Author

mtias commented Jun 26, 2017

@swissspidy ah, yes, definitely.

@swissspidy swissspidy mentioned this pull request Jun 26, 2017
@jasmussen jasmussen removed their assignment Jun 27, 2017
@jasmussen
Copy link
Contributor

I pushed a little polish to this one. Now the text area doesn't span the full width. I also tweaked the description a bit.

I can't think of a better way to design this block so input is editable. It's not as discoverable as I'd like it to be, but I think we should get this in and iterate later.

Should we add a placeholder? Right now if you delete the text, it's just a dashed line.

@swissspidy
Copy link
Member

Should we add a placeholder? Right now if you delete the text, it's just a dashed line.

I think so. "Read more" makes sense. Looking at get_the_content(), (more…) seems to be the default for the link. However, that doesn't really work well as a placeholder.

jasmussen added a commit that referenced this pull request Jun 28, 2017
This is pending more improvements happening separately in #1440 and #1523.
jasmussen added a commit that referenced this pull request Jun 29, 2017
This is pending more improvements happening separately in #1440 and #1523.
mtias pushed a commit that referenced this pull request Jun 29, 2017
* Crop gallery images by default

This is a work in progress. Pushing so as to test in IE.

* Polish inspector controls generally.

This is pending more improvements happening separately in #1440 and #1523.

* Make the crop actually work.

* Enable crop by default. Props @mtias.

* Move to is-cropped classname.

* Fix for IE11.

* Revert back to object-fit.
/>
{ focus &&
<InspectorControls key="inspector">
<p className="editor-block-inspector__description">"More" allows you to break your post into a part shown on index pages, and the subsequent after clicking a "Read More" link.</p>
Copy link
Member

Choose a reason for hiding this comment

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

This could use the new <BlockDescription> element

@westonruter
Copy link
Member

Can we somehow enforce that this block only can be used once in a post? Something for a separate PR, but just thinking out loud here.

See #1661.

@youknowriad
Copy link
Contributor

I pushed some changes here (essentially the useOnce and some polish). I think this is ready to go, can we have some 👀 here?

@codecov-io
Copy link

codecov-io commented Jul 27, 2017

Codecov Report

Merging #1440 into master will increase coverage by 0.05%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1440      +/-   ##
==========================================
+ Coverage   18.82%   18.87%   +0.05%     
==========================================
  Files         129      130       +1     
  Lines        4197     4207      +10     
  Branches      716      718       +2     
==========================================
+ Hits          790      794       +4     
- Misses       2868     2873       +5     
- Partials      539      540       +1
Impacted Files Coverage Δ
blocks/api/serializer.js 100% <100%> (+3.03%) ⬆️
blocks/library/more/index.js 30% <30%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b4a30ac...e7f04b0. Read the comment docs.

@youknowriad
Copy link
Contributor

Ok merging. We can iterate in smaller PRs

@youknowriad youknowriad merged commit 0f10ac1 into master Jul 27, 2017
@youknowriad youknowriad deleted the add/more-block branch July 27, 2017 14:16
@youknowriad youknowriad mentioned this pull request Jul 27, 2017
@swissspidy
Copy link
Member

Maybe another invalid variation such as <!--more-->\nsome text<!--noteaser--> - how likely is this to happen?

It's not invalid in PHP. WordPress doesn't care where in the content <!--noteaser--> is. It doesn't need to follow right after the more tag. How should legacy content be handled in this scenario?

@ellatrix
Copy link
Member

ellatrix commented Jul 28, 2017

I haven't followed the discussion closely, and maybe I'm just repeating what someone else said, but what if we put the legacy content in the block body, and upgrade old ones when they're between blocks?

<!-- wp:core/more { "text": "some text...", "noTeaser": true } -->
<!--more some text...-->
<!--noteaser-->
<!-- /wp:core/more -->

@aduth
Copy link
Member

aduth commented Jul 28, 2017

@iseulde I'm not really sure. I like your proposal for consistency's sake. Maybe there's an argument for special-casing some "known" types.

@nylen
Copy link
Member

nylen commented Jul 31, 2017

Another way that would be cleaner: make core recognize the <!-- wp:core/more /--> syntax using our server-side parser. I'm not sure how difficult this would be, but if it's achievable then we could just remove the old tags.

@mtias
Copy link
Member Author

mtias commented Aug 1, 2017

I agree with @nylen, let's see if we can consolidate all comments under the same syntax. Doesn't make much sense to me to wrap an old comment in new comments, it just adds noise.

@swissspidy swissspidy mentioned this pull request Oct 2, 2018
6 tasks
youknowriad pushed a commit that referenced this pull request Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants