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/block breadcrumb component readme #24827

Merged

Conversation

JustinyAhin
Copy link
Member

@JustinyAhin JustinyAhin commented Aug 26, 2020

Description

Added documentation for the block breadcrumb component (see #22891)

How has this been tested?

N/A, documentation only

Types of changes

Documentation

Checklist:

  • [n/a] My code is tested.
  • [n/a] My code follows the WordPress code style.
  • [n/a] My code follows the accessibility standards.
  • [n/a] My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@JustinyAhin JustinyAhin added the [Type] Developer Documentation Documentation for developers label Aug 26, 2020
<div className="add-your-class-name">
<BlockBreadcrumb />
</div>
```
Copy link
Contributor

Choose a reason for hiding this comment

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

I think most components in the block-editor package including this one need to be embedded in the React tree under BlockEditorProvider. I wonder if it's worth mentioning in the READMEs of the different components or not.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think we should include in each example, because a developer may approach any page without seeing others, the more we can do to make the examples "just work" when copy-and-paste the better.

The example could be just updated to, if you want to include the <div> it'd be fine too, but probably not necessary.

import { 
  BlockBreadcrumb,
  BlockEditorProvider  } from '@wordpress/block-editor';

<BlockEditorProvider>
    <BlockBreadcrumb/>
</BlockEditorProvider>

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not sufficient though because using BlockEditorProvider is more complex than that. Maybe we can link to its README?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, just updated the example with BlockEditorProvider.
Is there anything thing else that should be added?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe add something in a "Related components" section?

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally think we should revert to your original code example but add a sentence like that somewhere:

Block Editor components are components that can be used to compose the UI of your block editor. Thus, they can only be used under a `BlockEditorProvider` in the components tree. 

and link the word BlockEditorProvider to its README.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Works for me. The BlockEditorProvider readme has a complete example 👍

@youknowriad youknowriad requested a review from mkaz August 26, 2020 17:09

<div className="add-your-class-name">
<BlockBreadcrumb />
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is bad code, it needs to be wrapper inside a function (component), we can't just use JSX top level like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

something like
const MyBreadcrumb = () => <BlockBreadcrumb />; I guess.

I think I can also remove the div.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@youknowriad youknowriad merged commit f393474 into WordPress:master Aug 28, 2020
@github-actions github-actions bot added this to the Gutenberg 8.9 milestone Aug 28, 2020
@JustinyAhin JustinyAhin deleted the add/block-breadcrumb-component-readme branch August 31, 2020 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants