From 880d127f21b0b168edb275d2afa2aa86589d0568 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 21 Nov 2019 13:46:19 +0000 Subject: [PATCH 01/17] Utilise view context to ensure access to all post types. --- packages/core-data/src/entities.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/core-data/src/entities.js b/packages/core-data/src/entities.js index 2e8ae68a88dd41..7ed6f76576383e 100644 --- a/packages/core-data/src/entities.js +++ b/packages/core-data/src/entities.js @@ -80,7 +80,13 @@ export const kinds = [ * @return {Promise} Entities promise */ function* loadPostTypeEntities() { - const postTypes = yield apiFetch( { path: '/wp/v2/types?context=edit' } ); + // Setting "context" to "view" to work around bug whereby + // get_items_permissions_check in `WP_REST_Post_Types_Controller` + // does not pass for the "edit" context because it only checks + // for `edit_posts` permission + // wordpress/src/wp-includes/rest-api/endpoints/class-wp-rest-post-types-controller.php + const postTypes = yield apiFetch( { path: '/wp/v2/types?context=view' } ); + return map( postTypes, ( postType, name ) => { return { kind: 'postType', From 24a1fec3a170c05e2c009a225b73ef6dea8414e5 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 21 Nov 2019 14:24:37 +0000 Subject: [PATCH 02/17] Allow context to be overidden in query args --- packages/core-data/src/resolvers.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index b492a63e67e99e..5af23eef3aa73c 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -70,14 +70,16 @@ export function* getEntityRecord( kind, name, key = '' ) { */ export function* getEntityRecords( kind, name, query = {} ) { const entities = yield getKindEntities( kind ); + const entity = find( entities, { kind, name } ); if ( ! entity ) { return; } const path = addQueryArgs( entity.baseURL, { - ...query, context: 'edit', + ...query, } ); + const records = yield apiFetch( { path } ); yield receiveEntityRecords( kind, name, Object.values( records ), query ); } From b8465d78b1d94413e2df0f5c8657b65d000ede23 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 21 Nov 2019 14:25:02 +0000 Subject: [PATCH 03/17] =?UTF-8?q?Set=20Nav=20to=20use=20=E2=80=9Cview?= =?UTF-8?q?=E2=80=9D=20REST=20API=20context?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/block-library/src/navigation/edit.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/block-library/src/navigation/edit.js b/packages/block-library/src/navigation/edit.js index 416ce83431f636..a97e0a96fe2911 100644 --- a/packages/block-library/src/navigation/edit.js +++ b/packages/block-library/src/navigation/edit.js @@ -294,6 +294,7 @@ export default compose( [ parent: 0, order: 'asc', orderby: 'id', + context: 'view', }; const pagesSelect = [ From 46b09807d12c461228d921105d3895e7afda0a79 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 21 Nov 2019 15:52:42 +0000 Subject: [PATCH 04/17] Revert "Utilise view context to ensure access to all post types." This reverts commit 83ff2c829cd6dfabe98a61f6787d9bc0a8e6dfcc. --- packages/core-data/src/entities.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/core-data/src/entities.js b/packages/core-data/src/entities.js index 7ed6f76576383e..2e8ae68a88dd41 100644 --- a/packages/core-data/src/entities.js +++ b/packages/core-data/src/entities.js @@ -80,13 +80,7 @@ export const kinds = [ * @return {Promise} Entities promise */ function* loadPostTypeEntities() { - // Setting "context" to "view" to work around bug whereby - // get_items_permissions_check in `WP_REST_Post_Types_Controller` - // does not pass for the "edit" context because it only checks - // for `edit_posts` permission - // wordpress/src/wp-includes/rest-api/endpoints/class-wp-rest-post-types-controller.php - const postTypes = yield apiFetch( { path: '/wp/v2/types?context=view' } ); - + const postTypes = yield apiFetch( { path: '/wp/v2/types?context=edit' } ); return map( postTypes, ( postType, name ) => { return { kind: 'postType', From 83b5c56ee1a9345c831583d849f9611c99a9ea77 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 21 Nov 2019 15:52:42 +0000 Subject: [PATCH 05/17] Revert "Allow context to be overidden in query args" This reverts commit f800f86f5cbb42e0294e914718917e6416f8f566. --- packages/core-data/src/resolvers.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 5af23eef3aa73c..b492a63e67e99e 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -70,16 +70,14 @@ export function* getEntityRecord( kind, name, key = '' ) { */ export function* getEntityRecords( kind, name, query = {} ) { const entities = yield getKindEntities( kind ); - const entity = find( entities, { kind, name } ); if ( ! entity ) { return; } const path = addQueryArgs( entity.baseURL, { - context: 'edit', ...query, + context: 'edit', } ); - const records = yield apiFetch( { path } ); yield receiveEntityRecords( kind, name, Object.values( records ), query ); } From 07a0821489a0fbf314847db74ce1ef470b0af4fd Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 21 Nov 2019 15:52:42 +0000 Subject: [PATCH 06/17] =?UTF-8?q?Revert=20"Set=20Nav=20to=20use=20?= =?UTF-8?q?=E2=80=9Cview=E2=80=9D=20REST=20API=20context"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit d77bd469ad17d3aecfb145398567e818e73de817. --- packages/block-library/src/navigation/edit.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/block-library/src/navigation/edit.js b/packages/block-library/src/navigation/edit.js index a97e0a96fe2911..416ce83431f636 100644 --- a/packages/block-library/src/navigation/edit.js +++ b/packages/block-library/src/navigation/edit.js @@ -294,7 +294,6 @@ export default compose( [ parent: 0, order: 'asc', orderby: 'id', - context: 'view', }; const pagesSelect = [ From 013d7597d16eabbdbf695df9e68162facc38cfe8 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 21 Nov 2019 16:11:29 +0000 Subject: [PATCH 07/17] Initial naive refactor to use apiFetch directly. --- packages/block-library/src/navigation/edit.js | 224 ++++++++++-------- 1 file changed, 131 insertions(+), 93 deletions(-) diff --git a/packages/block-library/src/navigation/edit.js b/packages/block-library/src/navigation/edit.js index 416ce83431f636..1829a5069ce7fa 100644 --- a/packages/block-library/src/navigation/edit.js +++ b/packages/block-library/src/navigation/edit.js @@ -7,7 +7,13 @@ import classnames from 'classnames'; /** * WordPress dependencies */ -import { useMemo, Fragment, useRef } from '@wordpress/element'; +import { + useMemo, + Fragment, + useRef, + useState, + useEffect, +} from '@wordpress/element'; import { InnerBlocks, InspectorControls, @@ -18,6 +24,8 @@ import { __experimentalBlock as Block, } from '@wordpress/block-editor'; +import apiFetch from '@wordpress/api-fetch'; + import { createBlock } from '@wordpress/blocks'; import { useDispatch, withSelect, withDispatch } from '@wordpress/data'; import { @@ -32,6 +40,7 @@ import { import { compose } from '@wordpress/compose'; import { __ } from '@wordpress/i18n'; import { menu } from '@wordpress/icons'; +import { addQueryArgs } from '@wordpress/url'; /** * Internal dependencies @@ -41,7 +50,7 @@ import BlockNavigationList from './block-navigation-list'; import BlockColorsStyleSelector from './block-colors-selector'; import * as navIcons from './icons'; -function Navigation( { +function Navigation({ attributes, clientId, fontSize, @@ -53,13 +62,13 @@ function Navigation( { setFontSize, updateNavItemBlocks, className, -} ) { +}) { // // HOOKS // /* eslint-disable @wordpress/no-unused-vars-before-return */ const ref = useRef(); - const { selectBlock } = useDispatch( 'core/block-editor' ); + const { selectBlock } = useDispatch('core/block-editor'); const { TextColor, @@ -84,62 +93,93 @@ function Navigation( { initialOpen: true, }, }, - [ fontSize.size ] + [fontSize.size] ); + const [pages, setPages] = useState(null); + /* eslint-enable @wordpress/no-unused-vars-before-return */ const { navigatorToolbarButton, navigatorModal } = useBlockNavigator( clientId ); + let isStillMounted = true; + // Builds navigation links from default Pages. - const defaultPagesNavigationItems = useMemo( () => { - if ( ! pages ) { + const defaultPagesNavigationItems = useMemo(() => { + if (!pages) { return null; } - return pages.map( ( { title, type, link: url, id } ) => - createBlock( 'core/navigation-link', { + return pages.map(({ title, type, link: url, id }) => + createBlock('core/navigation-link', { type, id, url, - label: ! title.rendered - ? __( '(no title)' ) - : escape( title.rendered ), + label: !title.rendered + ? __('(no title)') + : escape(title.rendered), opensInNewTab: false, - } ) + }) ); - }, [ pages ] ); + }, [pages]); + + useEffect(() => { + const baseUrl = '/wp/v2/pages'; + const filterDefaultPages = { + parent: 0, + order: 'asc', + orderby: 'id', + context: 'view', + }; + apiFetch({ + path: addQueryArgs(baseUrl, filterDefaultPages), + }) + .then((pagesList) => { + if (isStillMounted) { + setPages(pagesList); + } + }) + .catch(() => { + if (isStillMounted) { + setPages([]); + } + }); + + return () => { + isStillMounted = false; + }; + }, []); // // HANDLERS // - function handleItemsAlignment( align ) { + function handleItemsAlignment(align) { return () => { const itemsJustification = attributes.itemsJustification === align ? undefined : align; - setAttributes( { + setAttributes({ itemsJustification, - } ); + }); }; } function handleCreateEmpty() { - const emptyNavLinkBlock = createBlock( 'core/navigation-link' ); - updateNavItemBlocks( [ emptyNavLinkBlock ] ); + const emptyNavLinkBlock = createBlock('core/navigation-link'); + updateNavItemBlocks([emptyNavLinkBlock]); } function handleCreateFromExistingPages() { - updateNavItemBlocks( defaultPagesNavigationItems ); - selectBlock( clientId ); + updateNavItemBlocks(defaultPagesNavigationItems); + selectBlock(clientId); } const hasPages = hasResolvedPages && pages && pages.length; - const blockClassNames = classnames( className, { - [ `items-justified-${ attributes.itemsJustification }` ]: attributes.itemsJustification, - [ fontSize.class ]: fontSize.class, - } ); + const blockClassNames = classnames(className, { + [`items-justified-${attributes.itemsJustification}`]: attributes.itemsJustification, + [fontSize.class]: fontSize.class, + }); const blockInlineStyles = { fontSize: fontSize.size ? fontSize.size + 'px' : undefined, }; @@ -147,36 +187,36 @@ function Navigation( { // If we don't have existing items or the User hasn't // indicated they want to automatically add top level Pages // then show the Placeholder - if ( ! hasExistingNavItems ) { + if (!hasExistingNavItems) { return (
@@ -192,91 +232,91 @@ function Navigation( { icon={ attributes.itemsJustification ? navIcons[ - `justify${ upperFirst( + `justify${upperFirst( attributes.itemsJustification - ) }Icon` + )}Icon` ] : navIcons.justifyLeftIcon } - label={ __( 'Change items justification' ) } + label={__('Change items justification')} isCollapsed - controls={ [ + controls={[ { icon: navIcons.justifyLeftIcon, - title: __( 'Justify items left' ), + title: __('Justify items left'), isActive: 'left' === attributes.itemsJustification, - onClick: handleItemsAlignment( 'left' ), + onClick: handleItemsAlignment('left'), }, { icon: navIcons.justifyCenterIcon, - title: __( 'Justify items center' ), + title: __('Justify items center'), isActive: 'center' === attributes.itemsJustification, - onClick: handleItemsAlignment( 'center' ), + onClick: handleItemsAlignment('center'), }, { icon: navIcons.justifyRightIcon, - title: __( 'Justify items right' ), + title: __('Justify items right'), isActive: 'right' === attributes.itemsJustification, - onClick: handleItemsAlignment( 'right' ), + onClick: handleItemsAlignment('right'), }, - ] } + ]} /> - { navigatorToolbarButton } + {navigatorToolbarButton} - { ColorPanel } + {ColorPanel} - { navigatorModal } + {navigatorModal} - - + + - + - { InspectorControlsColorPanel } + {InspectorControlsColorPanel} - + { - setAttributes( { showSubmenuIcon: value } ); - } } - label={ __( 'Show submenu indicator icons' ) } + checked={attributes.showSubmenuIcon} + onChange={(value) => { + setAttributes({ showSubmenuIcon: value }); + }} + label={__('Show submenu indicator icons')} /> - { ! hasExistingNavItems && isRequestingPages && ( + {!hasExistingNavItems && isRequestingPages && ( <> - { __( 'Loading Navigation…' ) }{ ' ' } + {__('Loading Navigation…')}{' '} - ) } + )} @@ -285,10 +325,10 @@ function Navigation( { ); } -export default compose( [ - withFontSizes( 'fontSize' ), - withSelect( ( select, { clientId } ) => { - const innerBlocks = select( 'core/block-editor' ).getBlocks( clientId ); +export default compose([ + withFontSizes('fontSize'), + withSelect((select, { clientId }) => { + const innerBlocks = select('core/block-editor').getBlocks(clientId); const filterDefaultPages = { parent: 0, @@ -299,32 +339,30 @@ export default compose( [ const pagesSelect = [ 'core', 'getEntityRecords', - [ 'postType', 'page', filterDefaultPages ], + ['postType', 'page', filterDefaultPages], ]; return { - hasExistingNavItems: !! innerBlocks.length, - pages: select( 'core' ).getEntityRecords( + hasExistingNavItems: !!innerBlocks.length, + pages: select('core').getEntityRecords( 'postType', 'page', filterDefaultPages ), - isRequestingPages: select( 'core/data' ).isResolving( - ...pagesSelect - ), - hasResolvedPages: select( 'core/data' ).hasFinishedResolution( + isRequestingPages: select('core/data').isResolving(...pagesSelect), + hasResolvedPages: select('core/data').hasFinishedResolution( ...pagesSelect ), }; - } ), - withDispatch( ( dispatch, { clientId } ) => { + }), + withDispatch((dispatch, { clientId }) => { return { - updateNavItemBlocks( blocks ) { - dispatch( 'core/block-editor' ).replaceInnerBlocks( + updateNavItemBlocks(blocks) { + dispatch('core/block-editor').replaceInnerBlocks( clientId, blocks ); }, }; - } ), -] )( Navigation ); + }), +])(Navigation); From d9287c5d3856d378fe92038338c2bcc8f210b8bd Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 21 Nov 2019 16:32:58 +0000 Subject: [PATCH 08/17] Improves handling of API request and adds comments --- packages/block-library/src/navigation/edit.js | 65 +++++++++++-------- 1 file changed, 38 insertions(+), 27 deletions(-) diff --git a/packages/block-library/src/navigation/edit.js b/packages/block-library/src/navigation/edit.js index 1829a5069ce7fa..51dc4ebaaac7e5 100644 --- a/packages/block-library/src/navigation/edit.js +++ b/packages/block-library/src/navigation/edit.js @@ -103,7 +103,8 @@ function Navigation({ clientId ); - let isStillMounted = true; + let isMounted = true; + let isRequestingPages = false; // Builds navigation links from default Pages. const defaultPagesNavigationItems = useMemo(() => { @@ -125,29 +126,43 @@ function Navigation({ }, [pages]); useEffect(() => { + // Indicate the fetching status + isRequestingPages = true; + const baseUrl = '/wp/v2/pages'; + + // "view" is required to ensure Pages are returned by REST API + // for users with lower capabilities such as "Contributor" otherwise + // Pages are not returned in the request if "edit" context is set + const context = 'view'; + const filterDefaultPages = { parent: 0, order: 'asc', orderby: 'id', - context: 'view', + context, }; + apiFetch({ path: addQueryArgs(baseUrl, filterDefaultPages), }) .then((pagesList) => { - if (isStillMounted) { + if (isMounted) { setPages(pagesList); } + // We've stopped fetching + isRequestingPages = false; }) .catch(() => { - if (isStillMounted) { + if (isMounted) { setPages([]); } + // We've stopped fetching + isRequestingPages = false; }); return () => { - isStillMounted = false; + isMounted = false; }; }, []); @@ -174,7 +189,7 @@ function Navigation({ selectBlock(clientId); } - const hasPages = hasResolvedPages && pages && pages.length; + const hasPages = pages && pages.length; const blockClassNames = classnames(className, { [`items-justified-${attributes.itemsJustification}`]: attributes.itemsJustification, @@ -190,6 +205,23 @@ function Navigation({ if (!hasExistingNavItems) { return ( + + {hasPages && ( + + { + setAttributes({ automaticallyAdd }); + handleCreateFromExistingPages(); + }} + label={__('Automatically add new pages')} + help={__( + 'Automatically add new top level pages to this navigation.' + )} + /> + + )} + { const innerBlocks = select('core/block-editor').getBlocks(clientId); - const filterDefaultPages = { - parent: 0, - order: 'asc', - orderby: 'id', - }; - - const pagesSelect = [ - 'core', - 'getEntityRecords', - ['postType', 'page', filterDefaultPages], - ]; - return { hasExistingNavItems: !!innerBlocks.length, - pages: select('core').getEntityRecords( - 'postType', - 'page', - filterDefaultPages - ), - isRequestingPages: select('core/data').isResolving(...pagesSelect), - hasResolvedPages: select('core/data').hasFinishedResolution( - ...pagesSelect - ), }; }), withDispatch((dispatch, { clientId }) => { From 55bf64c41d17d29694296dddb1d6dc4448868ce0 Mon Sep 17 00:00:00 2001 From: tellthemachines Date: Wed, 25 Mar 2020 10:42:13 +1100 Subject: [PATCH 09/17] Fix errors from conflict solving. --- packages/block-library/src/navigation/edit.js | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/packages/block-library/src/navigation/edit.js b/packages/block-library/src/navigation/edit.js index 51dc4ebaaac7e5..6c2a9b5ba67f2c 100644 --- a/packages/block-library/src/navigation/edit.js +++ b/packages/block-library/src/navigation/edit.js @@ -55,9 +55,6 @@ function Navigation({ clientId, fontSize, hasExistingNavItems, - hasResolvedPages, - isRequestingPages, - pages, setAttributes, setFontSize, updateNavItemBlocks, @@ -205,23 +202,6 @@ function Navigation({ if (!hasExistingNavItems) { return ( - - {hasPages && ( - - { - setAttributes({ automaticallyAdd }); - handleCreateFromExistingPages(); - }} - label={__('Automatically add new pages')} - help={__( - 'Automatically add new top level pages to this navigation.' - )} - /> - - )} - Date: Wed, 25 Mar 2020 11:08:44 +1100 Subject: [PATCH 10/17] lint --- packages/block-library/src/navigation/edit.js | 200 +++++++++--------- 1 file changed, 100 insertions(+), 100 deletions(-) diff --git a/packages/block-library/src/navigation/edit.js b/packages/block-library/src/navigation/edit.js index 6c2a9b5ba67f2c..458084206189a6 100644 --- a/packages/block-library/src/navigation/edit.js +++ b/packages/block-library/src/navigation/edit.js @@ -50,7 +50,7 @@ import BlockNavigationList from './block-navigation-list'; import BlockColorsStyleSelector from './block-colors-selector'; import * as navIcons from './icons'; -function Navigation({ +function Navigation( { attributes, clientId, fontSize, @@ -59,13 +59,13 @@ function Navigation({ setFontSize, updateNavItemBlocks, className, -}) { +} ) { // // HOOKS // /* eslint-disable @wordpress/no-unused-vars-before-return */ const ref = useRef(); - const { selectBlock } = useDispatch('core/block-editor'); + const { selectBlock } = useDispatch( 'core/block-editor' ); const { TextColor, @@ -90,10 +90,10 @@ function Navigation({ initialOpen: true, }, }, - [fontSize.size] + [ fontSize.size ] ); - const [pages, setPages] = useState(null); + const [ pages, setPages ] = useState( null ); /* eslint-enable @wordpress/no-unused-vars-before-return */ const { navigatorToolbarButton, navigatorModal } = useBlockNavigator( @@ -104,25 +104,25 @@ function Navigation({ let isRequestingPages = false; // Builds navigation links from default Pages. - const defaultPagesNavigationItems = useMemo(() => { - if (!pages) { + const defaultPagesNavigationItems = useMemo( () => { + if ( ! pages ) { return null; } - return pages.map(({ title, type, link: url, id }) => - createBlock('core/navigation-link', { + return pages.map( ( { title, type, link: url, id } ) => + createBlock( 'core/navigation-link', { type, id, url, - label: !title.rendered - ? __('(no title)') - : escape(title.rendered), + label: ! title.rendered + ? __( '(no title)' ) + : escape( title.rendered ), opensInNewTab: false, - }) + } ) ); - }, [pages]); + }, [ pages ] ); - useEffect(() => { + useEffect( () => { // Indicate the fetching status isRequestingPages = true; @@ -140,58 +140,58 @@ function Navigation({ context, }; - apiFetch({ - path: addQueryArgs(baseUrl, filterDefaultPages), - }) - .then((pagesList) => { - if (isMounted) { - setPages(pagesList); + apiFetch( { + path: addQueryArgs( baseUrl, filterDefaultPages ), + } ) + .then( ( pagesList ) => { + if ( isMounted ) { + setPages( pagesList ); } // We've stopped fetching isRequestingPages = false; - }) - .catch(() => { - if (isMounted) { - setPages([]); + } ) + .catch( () => { + if ( isMounted ) { + setPages( [] ); } // We've stopped fetching isRequestingPages = false; - }); + } ); return () => { isMounted = false; }; - }, []); + }, [] ); // // HANDLERS // - function handleItemsAlignment(align) { + function handleItemsAlignment( align ) { return () => { const itemsJustification = attributes.itemsJustification === align ? undefined : align; - setAttributes({ + setAttributes( { itemsJustification, - }); + } ); }; } function handleCreateEmpty() { - const emptyNavLinkBlock = createBlock('core/navigation-link'); - updateNavItemBlocks([emptyNavLinkBlock]); + const emptyNavLinkBlock = createBlock( 'core/navigation-link' ); + updateNavItemBlocks( [ emptyNavLinkBlock ] ); } function handleCreateFromExistingPages() { - updateNavItemBlocks(defaultPagesNavigationItems); - selectBlock(clientId); + updateNavItemBlocks( defaultPagesNavigationItems ); + selectBlock( clientId ); } const hasPages = pages && pages.length; - const blockClassNames = classnames(className, { - [`items-justified-${attributes.itemsJustification}`]: attributes.itemsJustification, - [fontSize.class]: fontSize.class, - }); + const blockClassNames = classnames( className, { + [ `items-justified-${ attributes.itemsJustification }` ]: attributes.itemsJustification, + [ fontSize.class ]: fontSize.class, + } ); const blockInlineStyles = { fontSize: fontSize.size ? fontSize.size + 'px' : undefined, }; @@ -199,36 +199,36 @@ function Navigation({ // If we don't have existing items or the User hasn't // indicated they want to automatically add top level Pages // then show the Placeholder - if (!hasExistingNavItems) { + if ( ! hasExistingNavItems ) { return (
@@ -244,91 +244,91 @@ function Navigation({ icon={ attributes.itemsJustification ? navIcons[ - `justify${upperFirst( + `justify${ upperFirst( attributes.itemsJustification - )}Icon` + ) }Icon` ] : navIcons.justifyLeftIcon } - label={__('Change items justification')} + label={ __( 'Change items justification' ) } isCollapsed - controls={[ + controls={ [ { icon: navIcons.justifyLeftIcon, - title: __('Justify items left'), + title: __( 'Justify items left' ), isActive: 'left' === attributes.itemsJustification, - onClick: handleItemsAlignment('left'), + onClick: handleItemsAlignment( 'left' ), }, { icon: navIcons.justifyCenterIcon, - title: __('Justify items center'), + title: __( 'Justify items center' ), isActive: 'center' === attributes.itemsJustification, - onClick: handleItemsAlignment('center'), + onClick: handleItemsAlignment( 'center' ), }, { icon: navIcons.justifyRightIcon, - title: __('Justify items right'), + title: __( 'Justify items right' ), isActive: 'right' === attributes.itemsJustification, - onClick: handleItemsAlignment('right'), + onClick: handleItemsAlignment( 'right' ), }, - ]} + ] } /> - {navigatorToolbarButton} + { navigatorToolbarButton } - {ColorPanel} + { ColorPanel } - {navigatorModal} + { navigatorModal } - - + + - + - {InspectorControlsColorPanel} + { InspectorControlsColorPanel } - + { - setAttributes({ showSubmenuIcon: value }); - }} - label={__('Show submenu indicator icons')} + checked={ attributes.showSubmenuIcon } + onChange={ ( value ) => { + setAttributes( { showSubmenuIcon: value } ); + } } + label={ __( 'Show submenu indicator icons' ) } /> - {!hasExistingNavItems && isRequestingPages && ( + { ! hasExistingNavItems && isRequestingPages && ( <> - {__('Loading Navigation…')}{' '} + { __( 'Loading Navigation…' ) }{ ' ' } - )} + ) } @@ -337,23 +337,23 @@ function Navigation({ ); } -export default compose([ - withFontSizes('fontSize'), - withSelect((select, { clientId }) => { - const innerBlocks = select('core/block-editor').getBlocks(clientId); +export default compose( [ + withFontSizes( 'fontSize' ), + withSelect( ( select, { clientId } ) => { + const innerBlocks = select( 'core/block-editor' ).getBlocks( clientId ); return { - hasExistingNavItems: !!innerBlocks.length, + hasExistingNavItems: !! innerBlocks.length, }; - }), - withDispatch((dispatch, { clientId }) => { + } ), + withDispatch( ( dispatch, { clientId } ) => { return { - updateNavItemBlocks(blocks) { - dispatch('core/block-editor').replaceInnerBlocks( + updateNavItemBlocks( blocks ) { + dispatch( 'core/block-editor' ).replaceInnerBlocks( clientId, blocks ); }, }; - }), -])(Navigation); + } ), +] )( Navigation ); From 8bbe13a796886242cf1df41c42b2b930eefea9da Mon Sep 17 00:00:00 2001 From: tellthemachines Date: Wed, 25 Mar 2020 12:35:33 +1100 Subject: [PATCH 11/17] Extracted fetch hook --- packages/block-library/src/navigation/edit.js | 71 ++++++------------- .../src/navigation/use-api-fetch.js | 24 +++++++ 2 files changed, 44 insertions(+), 51 deletions(-) create mode 100644 packages/block-library/src/navigation/use-api-fetch.js diff --git a/packages/block-library/src/navigation/edit.js b/packages/block-library/src/navigation/edit.js index 458084206189a6..9df8b1c2ac9caa 100644 --- a/packages/block-library/src/navigation/edit.js +++ b/packages/block-library/src/navigation/edit.js @@ -7,13 +7,7 @@ import classnames from 'classnames'; /** * WordPress dependencies */ -import { - useMemo, - Fragment, - useRef, - useState, - useEffect, -} from '@wordpress/element'; +import { useMemo, Fragment, useRef, useState } from '@wordpress/element'; import { InnerBlocks, InspectorControls, @@ -24,8 +18,6 @@ import { __experimentalBlock as Block, } from '@wordpress/block-editor'; -import apiFetch from '@wordpress/api-fetch'; - import { createBlock } from '@wordpress/blocks'; import { useDispatch, withSelect, withDispatch } from '@wordpress/data'; import { @@ -49,6 +41,7 @@ import useBlockNavigator from './use-block-navigator'; import BlockNavigationList from './block-navigation-list'; import BlockColorsStyleSelector from './block-colors-selector'; import * as navIcons from './icons'; +import useApiFetch from './use-api-fetch'; function Navigation( { attributes, @@ -63,10 +56,10 @@ function Navigation( { // // HOOKS // - /* eslint-disable @wordpress/no-unused-vars-before-return */ const ref = useRef(); const { selectBlock } = useDispatch( 'core/block-editor' ); + /* eslint-disable @wordpress/no-unused-vars-before-return */ const { TextColor, BackgroundColor, @@ -92,16 +85,15 @@ function Navigation( { }, [ fontSize.size ] ); - - const [ pages, setPages ] = useState( null ); - /* eslint-enable @wordpress/no-unused-vars-before-return */ + const { navigatorToolbarButton, navigatorModal } = useBlockNavigator( clientId ); - let isMounted = true; - let isRequestingPages = false; + const [ pages, setPages ] = useState( null ); + + const [ isRequestingPages, setIsRequestingPages ] = useState( false ); // Builds navigation links from default Pages. const defaultPagesNavigationItems = useMemo( () => { @@ -122,46 +114,23 @@ function Navigation( { ); }, [ pages ] ); - useEffect( () => { - // Indicate the fetching status - isRequestingPages = true; - - const baseUrl = '/wp/v2/pages'; + const baseUrl = '/wp/v2/pages'; - // "view" is required to ensure Pages are returned by REST API - // for users with lower capabilities such as "Contributor" otherwise - // Pages are not returned in the request if "edit" context is set - const context = 'view'; + // "view" is required to ensure Pages are returned by REST API + // for users with lower capabilities such as "Contributor" otherwise + // Pages are not returned in the request if "edit" context is set + const context = 'view'; - const filterDefaultPages = { - parent: 0, - order: 'asc', - orderby: 'id', - context, - }; + const filterDefaultPages = { + parent: 0, + order: 'asc', + orderby: 'id', + context, + }; - apiFetch( { - path: addQueryArgs( baseUrl, filterDefaultPages ), - } ) - .then( ( pagesList ) => { - if ( isMounted ) { - setPages( pagesList ); - } - // We've stopped fetching - isRequestingPages = false; - } ) - .catch( () => { - if ( isMounted ) { - setPages( [] ); - } - // We've stopped fetching - isRequestingPages = false; - } ); + const queryPath = addQueryArgs( baseUrl, filterDefaultPages ); - return () => { - isMounted = false; - }; - }, [] ); + useApiFetch( setIsRequestingPages, setPages, queryPath ); // // HANDLERS diff --git a/packages/block-library/src/navigation/use-api-fetch.js b/packages/block-library/src/navigation/use-api-fetch.js new file mode 100644 index 00000000000000..36db58619ff79b --- /dev/null +++ b/packages/block-library/src/navigation/use-api-fetch.js @@ -0,0 +1,24 @@ +/** + * WordPress dependencies + */ +import { useEffect } from '@wordpress/element'; +import apiFetch from '@wordpress/api-fetch'; + +export default function useApiFetch( setIsRequesting, setData, path ) { + useEffect( () => { + // Indicate the fetching status + setIsRequesting( true ); + + apiFetch( { path } ) + .then( ( dataList ) => { + setData( dataList ); + // We've stopped fetching + setIsRequesting( false ); + } ) + .catch( () => { + setData( [] ); + // We've stopped fetching + setIsRequesting( false ); + } ); + }, [] ); +} From 90f92faa629ca4f4040dd79e0097e28aa4a889cf Mon Sep 17 00:00:00 2001 From: tellthemachines Date: Wed, 25 Mar 2020 12:39:55 +1100 Subject: [PATCH 12/17] Added jsdoc --- packages/block-library/src/navigation/use-api-fetch.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/block-library/src/navigation/use-api-fetch.js b/packages/block-library/src/navigation/use-api-fetch.js index 36db58619ff79b..6d51ff3e97bce0 100644 --- a/packages/block-library/src/navigation/use-api-fetch.js +++ b/packages/block-library/src/navigation/use-api-fetch.js @@ -4,6 +4,13 @@ import { useEffect } from '@wordpress/element'; import apiFetch from '@wordpress/api-fetch'; +/** + * Function that fetches data using apiFetch, and updates the status. + * + * @param {Function} setIsRequesting function to set requesting state to true or false. + * @param {Function} setData Function to set data being returned, or empty array on error. + * @param {string} path Query path. + */ export default function useApiFetch( setIsRequesting, setData, path ) { useEffect( () => { // Indicate the fetching status From dab1a15d6d7aacb99ab6894e4369b44b247e32c6 Mon Sep 17 00:00:00 2001 From: tellthemachines Date: Tue, 31 Mar 2020 13:30:14 +1100 Subject: [PATCH 13/17] Change API pattern. --- packages/block-library/src/navigation/edit.js | 40 +++++++++---------- .../src/navigation/use-api-fetch.js | 30 ++++++++------ 2 files changed, 37 insertions(+), 33 deletions(-) diff --git a/packages/block-library/src/navigation/edit.js b/packages/block-library/src/navigation/edit.js index 9df8b1c2ac9caa..811a4c8973061d 100644 --- a/packages/block-library/src/navigation/edit.js +++ b/packages/block-library/src/navigation/edit.js @@ -7,7 +7,7 @@ import classnames from 'classnames'; /** * WordPress dependencies */ -import { useMemo, Fragment, useRef, useState } from '@wordpress/element'; +import { useMemo, Fragment, useRef } from '@wordpress/element'; import { InnerBlocks, InspectorControls, @@ -91,9 +91,25 @@ function Navigation( { clientId ); - const [ pages, setPages ] = useState( null ); + const baseUrl = '/wp/v2/pages'; - const [ isRequestingPages, setIsRequestingPages ] = useState( false ); + // "view" is required to ensure Pages are returned by REST API + // for users with lower capabilities such as "Contributor" otherwise + // Pages are not returned in the request if "edit" context is set + const context = 'view'; + + const filterDefaultPages = { + parent: 0, + order: 'asc', + orderby: 'id', + context, + }; + + const queryPath = addQueryArgs( baseUrl, filterDefaultPages ); + + const { isLoading: isRequestingPages, data: pages } = useApiFetch( + queryPath + ); // Builds navigation links from default Pages. const defaultPagesNavigationItems = useMemo( () => { @@ -114,24 +130,6 @@ function Navigation( { ); }, [ pages ] ); - const baseUrl = '/wp/v2/pages'; - - // "view" is required to ensure Pages are returned by REST API - // for users with lower capabilities such as "Contributor" otherwise - // Pages are not returned in the request if "edit" context is set - const context = 'view'; - - const filterDefaultPages = { - parent: 0, - order: 'asc', - orderby: 'id', - context, - }; - - const queryPath = addQueryArgs( baseUrl, filterDefaultPages ); - - useApiFetch( setIsRequestingPages, setPages, queryPath ); - // // HANDLERS // diff --git a/packages/block-library/src/navigation/use-api-fetch.js b/packages/block-library/src/navigation/use-api-fetch.js index 6d51ff3e97bce0..10db2a3e2e6bc9 100644 --- a/packages/block-library/src/navigation/use-api-fetch.js +++ b/packages/block-library/src/navigation/use-api-fetch.js @@ -1,31 +1,37 @@ /** * WordPress dependencies */ -import { useEffect } from '@wordpress/element'; +import { useEffect, useState } from '@wordpress/element'; import apiFetch from '@wordpress/api-fetch'; /** * Function that fetches data using apiFetch, and updates the status. * - * @param {Function} setIsRequesting function to set requesting state to true or false. - * @param {Function} setData Function to set data being returned, or empty array on error. * @param {string} path Query path. */ -export default function useApiFetch( setIsRequesting, setData, path ) { - useEffect( () => { - // Indicate the fetching status - setIsRequesting( true ); +export default function useApiFetch( path ) { + // Indicate the fetching status + const [ isLoading, setIsLoading ] = useState( true ); + const [ data, setData ] = useState( [] ); + const [ error, setError ] = useState( null ); + useEffect( () => { apiFetch( { path } ) .then( ( dataList ) => { setData( dataList ); // We've stopped fetching - setIsRequesting( false ); + setIsLoading( false ); } ) - .catch( () => { - setData( [] ); + .catch( ( err ) => { + setError( err ); // We've stopped fetching - setIsRequesting( false ); + setIsLoading( false ); } ); - }, [] ); + }, [ path ] ); + + return { + isLoading, + data, + error, + }; } From 0a5d67fcc07e8821872f512b69781cc7a7303499 Mon Sep 17 00:00:00 2001 From: tellthemachines Date: Tue, 31 Mar 2020 13:34:30 +1100 Subject: [PATCH 14/17] Clean up booleans and conditions --- packages/block-library/src/navigation/edit.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/block-library/src/navigation/edit.js b/packages/block-library/src/navigation/edit.js index 811a4c8973061d..e0f6882734b147 100644 --- a/packages/block-library/src/navigation/edit.js +++ b/packages/block-library/src/navigation/edit.js @@ -111,9 +111,11 @@ function Navigation( { queryPath ); + const hasPages = !! pages && !! pages.length; + // Builds navigation links from default Pages. const defaultPagesNavigationItems = useMemo( () => { - if ( ! pages ) { + if ( ! hasPages ) { return null; } @@ -153,8 +155,6 @@ function Navigation( { selectBlock( clientId ); } - const hasPages = pages && pages.length; - const blockClassNames = classnames( className, { [ `items-justified-${ attributes.itemsJustification }` ]: attributes.itemsJustification, [ fontSize.class ]: fontSize.class, From 5b89402b884126c5f8ac3074a9fd837040c91c8b Mon Sep 17 00:00:00 2001 From: tellthemachines Date: Tue, 31 Mar 2020 14:41:39 +1100 Subject: [PATCH 15/17] Move hook to api-fetch package --- package-lock.json | 5 ++- packages/api-fetch/package.json | 1 + packages/api-fetch/src/index.js | 33 +++++++++++++++++ packages/block-library/src/navigation/edit.js | 3 +- .../src/navigation/use-api-fetch.js | 37 ------------------- 5 files changed, 38 insertions(+), 41 deletions(-) delete mode 100644 packages/block-library/src/navigation/use-api-fetch.js diff --git a/package-lock.json b/package-lock.json index e1c73beeb72079..7460d80bec9378 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10438,6 +10438,7 @@ "version": "file:packages/api-fetch", "requires": { "@babel/runtime": "^7.8.3", + "@wordpress/element": "file:packages/element", "@wordpress/i18n": "file:packages/i18n", "@wordpress/url": "file:packages/url" } @@ -20592,7 +20593,7 @@ }, "node-pre-gyp": { "version": "0.12.0", - "resolved": false, + "resolved": "https://registry.npmjs.org/node-pre-gyp/-/node-pre-gyp-0.12.0.tgz", "integrity": "sha512-4KghwV8vH5k+g2ylT+sLTjy5wmUOb9vPhnM8NHvRf9dHmnW/CndrFXy2aRPaPST6dugXSdHXfeaHQm77PIz/1A==", "dev": true, "optional": true, @@ -20611,7 +20612,7 @@ }, "nopt": { "version": "4.0.1", - "resolved": false, + "resolved": "https://registry.npmjs.org/nopt/-/nopt-4.0.1.tgz", "integrity": "sha1-0NRoWv1UFRk8jHUFYC0NF81kR00=", "dev": true, "optional": true, diff --git a/packages/api-fetch/package.json b/packages/api-fetch/package.json index 0e5f9c1a912b2e..bd94813f413495 100644 --- a/packages/api-fetch/package.json +++ b/packages/api-fetch/package.json @@ -23,6 +23,7 @@ "react-native": "src/index", "dependencies": { "@babel/runtime": "^7.8.3", + "@wordpress/element": "file:../element", "@wordpress/i18n": "file:../i18n", "@wordpress/url": "file:../url" }, diff --git a/packages/api-fetch/src/index.js b/packages/api-fetch/src/index.js index 9e7edc985709ed..ec878c5dcf5bb6 100644 --- a/packages/api-fetch/src/index.js +++ b/packages/api-fetch/src/index.js @@ -2,6 +2,7 @@ * WordPress dependencies */ import { __ } from '@wordpress/i18n'; +import { useEffect, useState } from '@wordpress/element'; /** * Internal dependencies @@ -166,3 +167,35 @@ apiFetch.fetchAllMiddleware = fetchAllMiddleware; apiFetch.mediaUploadMiddleware = mediaUploadMiddleware; export default apiFetch; + +/** + * Function that fetches data using apiFetch, and updates the status. + * + * @param {string} path Query path. + */ +export function useApiFetch( path ) { + // Indicate the fetching status + const [ isLoading, setIsLoading ] = useState( true ); + const [ data, setData ] = useState( [] ); + const [ error, setError ] = useState( null ); + + useEffect( () => { + apiFetch( { path } ) + .then( ( dataList ) => { + setData( dataList ); + // We've stopped fetching + setIsLoading( false ); + } ) + .catch( ( err ) => { + setError( err ); + // We've stopped fetching + setIsLoading( false ); + } ); + }, [ path ] ); + + return { + isLoading, + data, + error, + }; +} diff --git a/packages/block-library/src/navigation/edit.js b/packages/block-library/src/navigation/edit.js index e0f6882734b147..7c9881379f596c 100644 --- a/packages/block-library/src/navigation/edit.js +++ b/packages/block-library/src/navigation/edit.js @@ -33,7 +33,7 @@ import { compose } from '@wordpress/compose'; import { __ } from '@wordpress/i18n'; import { menu } from '@wordpress/icons'; import { addQueryArgs } from '@wordpress/url'; - +import { useApiFetch } from '@wordpress/api-fetch'; /** * Internal dependencies */ @@ -41,7 +41,6 @@ import useBlockNavigator from './use-block-navigator'; import BlockNavigationList from './block-navigation-list'; import BlockColorsStyleSelector from './block-colors-selector'; import * as navIcons from './icons'; -import useApiFetch from './use-api-fetch'; function Navigation( { attributes, diff --git a/packages/block-library/src/navigation/use-api-fetch.js b/packages/block-library/src/navigation/use-api-fetch.js deleted file mode 100644 index 10db2a3e2e6bc9..00000000000000 --- a/packages/block-library/src/navigation/use-api-fetch.js +++ /dev/null @@ -1,37 +0,0 @@ -/** - * WordPress dependencies - */ -import { useEffect, useState } from '@wordpress/element'; -import apiFetch from '@wordpress/api-fetch'; - -/** - * Function that fetches data using apiFetch, and updates the status. - * - * @param {string} path Query path. - */ -export default function useApiFetch( path ) { - // Indicate the fetching status - const [ isLoading, setIsLoading ] = useState( true ); - const [ data, setData ] = useState( [] ); - const [ error, setError ] = useState( null ); - - useEffect( () => { - apiFetch( { path } ) - .then( ( dataList ) => { - setData( dataList ); - // We've stopped fetching - setIsLoading( false ); - } ) - .catch( ( err ) => { - setError( err ); - // We've stopped fetching - setIsLoading( false ); - } ); - }, [ path ] ); - - return { - isLoading, - data, - error, - }; -} From 585a6dbcfac6aa311123bcffeb5d5210e3a42361 Mon Sep 17 00:00:00 2001 From: tellthemachines Date: Tue, 31 Mar 2020 16:37:49 +1100 Subject: [PATCH 16/17] Attach hook to default export --- packages/api-fetch/src/index.js | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/packages/api-fetch/src/index.js b/packages/api-fetch/src/index.js index ec878c5dcf5bb6..005e63530f20f0 100644 --- a/packages/api-fetch/src/index.js +++ b/packages/api-fetch/src/index.js @@ -157,23 +157,12 @@ function apiFetch( options ) { } ); } -apiFetch.use = registerMiddleware; -apiFetch.setFetchHandler = setFetchHandler; - -apiFetch.createNonceMiddleware = createNonceMiddleware; -apiFetch.createPreloadingMiddleware = createPreloadingMiddleware; -apiFetch.createRootURLMiddleware = createRootURLMiddleware; -apiFetch.fetchAllMiddleware = fetchAllMiddleware; -apiFetch.mediaUploadMiddleware = mediaUploadMiddleware; - -export default apiFetch; - /** * Function that fetches data using apiFetch, and updates the status. * * @param {string} path Query path. */ -export function useApiFetch( path ) { +function useApiFetch( path ) { // Indicate the fetching status const [ isLoading, setIsLoading ] = useState( true ); const [ data, setData ] = useState( [] ); @@ -199,3 +188,16 @@ export function useApiFetch( path ) { error, }; } + +apiFetch.use = registerMiddleware; +apiFetch.setFetchHandler = setFetchHandler; + +apiFetch.createNonceMiddleware = createNonceMiddleware; +apiFetch.createPreloadingMiddleware = createPreloadingMiddleware; +apiFetch.createRootURLMiddleware = createRootURLMiddleware; +apiFetch.fetchAllMiddleware = fetchAllMiddleware; +apiFetch.mediaUploadMiddleware = mediaUploadMiddleware; + +apiFetch.useApiFetch = useApiFetch; + +export default apiFetch; From 641ba9e2686fa28580176f4863c1601ff22529f8 Mon Sep 17 00:00:00 2001 From: tellthemachines Date: Wed, 1 Apr 2020 10:17:46 +1100 Subject: [PATCH 17/17] Reset state before fetching, don't guess data type --- packages/api-fetch/src/index.js | 10 +++++++--- packages/block-library/src/navigation/edit.js | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/api-fetch/src/index.js b/packages/api-fetch/src/index.js index 005e63530f20f0..64fc3ef09e0c33 100644 --- a/packages/api-fetch/src/index.js +++ b/packages/api-fetch/src/index.js @@ -165,13 +165,17 @@ function apiFetch( options ) { function useApiFetch( path ) { // Indicate the fetching status const [ isLoading, setIsLoading ] = useState( true ); - const [ data, setData ] = useState( [] ); + const [ data, setData ] = useState( null ); const [ error, setError ] = useState( null ); useEffect( () => { + setIsLoading( true ); + setData( null ); + setError( null ); + apiFetch( { path } ) - .then( ( dataList ) => { - setData( dataList ); + .then( ( fetchedData ) => { + setData( fetchedData ); // We've stopped fetching setIsLoading( false ); } ) diff --git a/packages/block-library/src/navigation/edit.js b/packages/block-library/src/navigation/edit.js index 7c9881379f596c..e43bace4ab0a1f 100644 --- a/packages/block-library/src/navigation/edit.js +++ b/packages/block-library/src/navigation/edit.js @@ -110,7 +110,7 @@ function Navigation( { queryPath ); - const hasPages = !! pages && !! pages.length; + const hasPages = !! pages; // Builds navigation links from default Pages. const defaultPagesNavigationItems = useMemo( () => {