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.

Sign up for GitHub

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

Jump to bottom

wp.element - export lazy and Suspense #14412

Merged
merged 20 commits into from
Mar 26, 2019
Merged

wp.element - export lazy and Suspense #14412

merged 20 commits into from
Mar 26, 2019

Conversation

adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Mar 13, 2019 •
edited by gziolo
Loading

Description

Add React.lazy and React.suspense to exports from wp.element.

Fixes #14155

Testing

Load Gutenberg and check the console for lazy and Suspense on wp.element.

Screenshot

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

aduth
aduth reviewed Mar 13, 2019
View reviewed changes
packages/element/src/react.js Outdated
/**
* Make React Hooks available
*
* @param {function} loadFunction A function that must call a dynamic import(). This must return a Promise which
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this was meant to be included here?

Copy link
Member Author

@adamsilverstein adamsilverstein Mar 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Right, fixed. Not sure why there are unrelated docs changes even after merging master and running npm run docs:build - am I missing a step?

ps. thanks for reviewing!

Copy link
Member

Choose a reason for hiding this comment

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

running npm run docs:build - am I missing a step?

Can you try it once more? The changes I would have expected with this erroneous DocBlock, but I expect should be largely cleared up if run in the current state of the branch. Would also explain the failing build.

Copy link
Member Author

Choose a reason for hiding this comment

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

grabbed readme from master, then re-ran - changes in 1466b25.

gziolo
gziolo reviewed Mar 14, 2019
View reviewed changes
packages/element/README.md Outdated Show resolved Hide resolved
packages/element/src/react.js Outdated
* @param {function} loadFunction A function that must call a dynamic import(). This must return a Promise which
* resolves to a module with a default export containing a React component.
*/
export { lazy };
Copy link
Member

Choose a reason for hiding this comment

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

@nosolosw had a good idea to add the link to the original function description in React docs. See #14425.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, i updated my PR to use this format in bb22466

@adamsilverstein adamsilverstein requested review from gziolo and aduth March 25, 2019 14:15
@gziolo
Copy link
Member

gziolo commented Mar 26, 2019

I update links to use React Top-Level API page.

We're setting __webpack_public_path__ at runtime.

https://webpack.js.org/guides/public-path/#on-the-fly

@TimothyBJacobs do you think that it makes sense to open a follow-up PR with some explanation of how lazy and Suspense can be used with WordPress as of today?

gziolo
gziolo approved these changes Mar 26, 2019
View reviewed changes
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

@adamsilverstein thanks for this addition 👍

@gziolo gziolo added the [Package] Element /packages/element label Mar 26, 2019
@gziolo gziolo added this to the 5.4 (Gutenberg) milestone Mar 26, 2019
@gziolo
Copy link
Member

gziolo commented Mar 26, 2019

I also updated CHANGELOG file with new API methods exposed. In addition, I included all hooks which weren't fully documented in the last release. See 00fe1c5.

@gziolo gziolo merged commit 186ce11 into master Mar 26, 2019
@gziolo gziolo deleted the add/lazy-suspense branch March 26, 2019 08:14
@gziolo gziolo added the [Type] Task Issues or PRs that have been broken down into an individual action to take label Mar 26, 2019
@TimothyBJacobs
Copy link
Member

@TimothyBJacobs do you think that it makes sense to open a follow-up PR with some explanation of how lazy and Suspense can be used with WordPress as of today?

Yep. This week is a bit hectic, but I can work on something for next week if that helps.

This was referenced Apr 1, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oandregal oandregal oandregal left review comments

@gziolo gziolo gziolo approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@noisysocks noisysocks Awaiting requested review from noisysocks

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

@aduth aduth Awaiting requested review from aduth

Assignees
No one assigned
Labels
[Package] Element /packages/element [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Milestone
5.4 (Gutenberg)
Development

Successfully merging this pull request may close these issues.
5 participants
@adamsilverstein @gziolo @TimothyBJacobs @oandregal @aduth