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

Block type: add support link #11330

Closed
wants to merge 1 commit into from
Closed

Block type: add support link #11330

wants to merge 1 commit into from

Conversation

simison
Copy link
Member

@simison simison commented Oct 31, 2018

Description

I've added a way to declare where block's support documentation lives, via a block type options.

The link would appear under block description either as an external or internal link.

During our block development, we've started adding links to support documentation directly in "description" option. It works for desktop visually but isn't obviously great declaratively since it's no more just "description" and also goes against Gutenberg design rules:

description: (
  <Fragment>
    <p>
      { __(
        'Simple Payments lets you create and embed credit and ' +
          'debit card payment buttons on your WordPress.com and ' +
          'Jetpack-enabled sites with minimal setup.'
      ) }
    </p>
    <ExternalLink href="https://support.wordpress.com/simple-payments/">
      { __( 'Support reference' ) }
    </ExternalLink>
  </Fragment>
)

Would be great to have a declarative way to add support documentation and make it easier to show in an optimal way in other places such as mobile devices. Something like:

support: {
  url: 'https://support.wordpress.com/simple-payments/',
  label: __( 'Support reference' ),
  external: true,
},

This is a quick prototype; if you think it's a good idea, I'm happy to look into what other work might be needed to adding such support.

Further ideas:

  • This could also support passing URL as a string:

    support: 'https://wordpress.org/gutenberg/handbook/'
    
  • Could support adding multiple links as an array

  • The label could be made optional, defaulting to something like "Support reference".

  • Instead of support, the option could also be called documentation.

  • URL could be passed through prependHTTP

How has this been tested?

WIP

Screenshots

screen shot 2018-10-31 at 22 22 57

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo
Copy link
Member

gziolo commented Feb 4, 2019

For me personally, it feels like plugins territory. I would love to hear feedback from others.

@jasmussen
Copy link
Contributor

I'm tempted to hear more use cases and examples of where this might make sense.

Honestly it feels like links like these should be less "cookie cutter", and rather be worked into the prose of the paragraph, like a simple hyperlink. But could be I'm missing something?

@gziolo
Copy link
Member

gziolo commented Feb 4, 2019

Honestly it feels like links like these should be less "cookie cutter", and rather be worked into the prose of the paragraph, like a simple hyperlink. But could be I'm missing something?

The description is text only at the moment, so it would be a raw link included...

@simison
Copy link
Member Author

simison commented Feb 4, 2019

The description is text only at the moment, so it would be a raw link included...

You mean it's supposed to be... 😅

https://github.com/Automattic/wp-calypso/blob/83f7629381613eb5de5403c29180a0e846ad1f53/client/gutenberg/extensions/markdown/index.js#L22-L29

it feels like plugins territory.

@gziolo I don't follow you — wanna elaborate what you mean?

@gziolo
Copy link
Member

gziolo commented Feb 4, 2019

it feels like plugins territory.

@gziolo I don't follow you — wanna elaborate what you mean?

I mean that they can use blocks.registerBlockType filter to override the description.

The description is text only at the moment, so it would be a raw link included...

You mean it's supposed to be... 😅

I assumed it's text only, but it looks like we never validate it :) It makes only this whole process easier.

/cc @youknowriad and @aduth - this is an interesting finding, it might mean that most of the fields we consider as string only might, in fact, accept components.

@aduth
Copy link
Member

aduth commented Feb 4, 2019

I've encountered this too in the past. It's quite easy to overlook, unfortunately.

// The supported value shape of content is currently limited to plain text
// strings. To avoid setting expectation that e.g. a WPElement could be
// supported, cast to a string.
content = String( content );

It makes me conflicted because I think we're better to set the expectations earlier than to have it break at a point in the future, but the contract we establish makes it clear that this is to be a string, regardless of what other values incidentally happen to work.

https://wordpress.org/gutenberg/handbook/designers-developers/developers/block-api/block-registration/#description-optional

@simison
Copy link
Member Author

simison commented Feb 4, 2019

I'm tempted to hear more use cases and examples of where this might make sense.

Perhaps also a good place for:

  • upsell links for a premium version of the block
  • if block integrates with external service, there would be "Manage account" / "Register account" -link.

I mean that they can use blocks.registerBlockType filter to override the description.

Why would block builder do that vs what we've done? Shouldn't description still be considered a string even when modified via filter?

@jasmussen
Copy link
Contributor

Those use cases are totally fair, thanks for elaborating, and it especially makes sense in light of the textfield not allowing links (or it shouldn't). I'm not entirely sure a separate link below the box is the right solution though... maybe it is? It feels, to repeat myself, cookie-cutter: this is where you put text, this is where you put a link. It's great to provide APIs that are limited so as to ensure accessibility and usability even for plugins, but I'd love a sanity check on these thoughts.

@sarahmonster sarahmonster added Needs Accessibility Feedback Need input from accessibility Needs Copy Review Needs review of user-facing copy (language, phrasing) labels Feb 5, 2019
@sarahmonster
Copy link
Member

❤️ Anything that makes documentation more findable for users when they need it is a 👍 from me. Having an explicit mechanism for plugin developers to link to their documentation will ideally encourage developers to use it more.

Ideally, this would be something we could expand in the future to provide inline help directly, rather than taking the user to a new site, making for a more seamless and uninterrupted flow.

I'd be in favour of keeping the link separate from the description. This has a few benefits:

  • a consistent place for users to look for help
  • easier to manage translations
  • more immediately visible when scanning the page
  • the description doesn't need to be reworded to support a help link (potentially making it longer)

Regarding wording, I'd opt for "Help" or "How-to" or even just "Documentation" rather than "Support reference" here. (Added tag for copy review.)

It would be great to get an accessibility perspective on this proposed change as well. (Added tag for accessibility feedback.)

@danjjohnson
Copy link

I second everything @sarahmonster said. I think "Help" or "Documentation" would be better than "Support Reference", and I think keeping it separate from the description would be best.

@jasmussen
Copy link
Contributor

It sounds like a consensus is emerging!

@jasmussen jasmussen removed the Needs Design Feedback Needs general design feedback. label Feb 5, 2019
@michelleweber
Copy link

michelleweber commented Feb 5, 2019

"Support reference" and "documentation" both sound a little... serious? They're not normal-speech words for most people. "Help" feels like it doesn't give me quite enough context re: what happens when I click -- does that open a chat window, an email, a webpage?

I'd love something that felt a little more relaxed but with a little more info, like "Setup details"/"Block details" if it needs to be short, or "How to use this block" or "Learn more about this block" if that can be accommodated.

@mapk
Copy link
Contributor

mapk commented Feb 6, 2019

I like "Learn more about this block" as pointed out by @michelleweber, or even "Learn more" would work for me.

I also like how it's displayed in the initial post... a link at the bottom of the description. My reasons are that it's right inline with what I'm reading, and that feels like the place I'd expect to see any help links if there were any.

@gziolo
Copy link
Member

gziolo commented Feb 6, 2019

@simison - we are working on defining the server-side friendly definition of blocks, there is RFC opened which would be a great place to include your proposal given that it seems like there is agreement that it would be a nice addition. We will need to have this new property documented so it should be win-win if it's taken into account there from the start.

@gziolo gziolo added the [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible label Feb 6, 2019
{
blockType.support.external ? (
<ExternalLink href={ blockType.support.url }>
{ blockType.support.label }
Copy link
Member Author

Choose a reason for hiding this comment

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

We prolly need to cast this as a string to be on the safe side.

Copy link
Member

Choose a reason for hiding this comment

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

We prolly need to cast this as a string to be on the safe side.

Did you still intend to make this change?

@simison
Copy link
Member Author

simison commented Feb 6, 2019

@gziolo nice, so since folks are in favour of this feature, I can add docs in this PR (to here) — or did you mean I should chime in at RTC proposal?

I'll also rename the default label to "Learn more" (remember it's just default and possible to override).

@gziolo
Copy link
Member

gziolo commented Feb 7, 2019

There is also:
https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/developers/block-api/block-registration.md

There might be more documents which mention Block API :)

At some point, we will have to include it in RFC, too. Ideally, the proposed structure can be defined inside a JSON file.

@gziolo gziolo removed the Needs Accessibility Feedback Need input from accessibility label Feb 7, 2019
@gziolo gziolo added this to the 5.2 (Gutenberg) milestone Feb 7, 2019
@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

How can we move this forward? From the conversation here, it seems there still needs to be implemented support for the default label?

Is it considered blocked by the Block Registration RFC (#13693) ?

{ hasSupportLink && (
<div className="editor-block-inspector__card-support">
{
blockType.support.external ? (
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain what makes a support link "external"? Is it based on the origin/hostname of the URL, and if so, is that not something we should programmatically determine on behalf of the block developer, than to impose on them?

{
blockType.support.external ? (
<ExternalLink href={ blockType.support.url }>
{ blockType.support.label }
Copy link
Member

Choose a reason for hiding this comment

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

We prolly need to cast this as a string to be on the safe side.

Did you still intend to make this change?

@draganescu
Copy link
Contributor

@simison @gziolo apparently this was a good idea which went stale. Can it still move forward?

@draganescu draganescu added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Nov 22, 2019
@youknowriad
Copy link
Contributor

Trying to triage PRs today. It seems that this PR is stale. I'm going to close it for now. Thanks all for your efforts.

@mtias I was also wondering what you think about this config? and whether it's worth considering refreshing this PR?

@mrfoxtalbot
Copy link

mrfoxtalbot commented Feb 3, 2022

I find that providing an inline support link right next to the block description could have a positive impact on the learning curve of new users. This would be particularly true in the case of blocks added by plugins, which can be considerably more complex than the ones in core.

Could we open this PR just to revisit this? Thank you!

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 Needs Copy Review Needs review of user-facing copy (language, phrasing) [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.