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

Fix Nav Block bug as Contributor user via fix to *loadPostTypeEntities #18669

Merged
merged 17 commits into from
Apr 1, 2020
Merged

Fix Nav Block bug as Contributor user via fix to *loadPostTypeEntities #18669

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
880d127
Utilise view context to ensure access to all post types.
getdave Nov 21, 2019
24a1fec
Allow context to be overidden in query args
getdave Nov 21, 2019
b8465d7
Set Nav to use “view” REST API context
getdave Nov 21, 2019
46b0980
Revert "Utilise view context to ensure access to all post types."
getdave Nov 21, 2019
83b5c56
Revert "Allow context to be overidden in query args"
getdave Nov 21, 2019
07a0821
Revert "Set Nav to use “view” REST API context"
getdave Nov 21, 2019
013d759
Initial naive refactor to use apiFetch directly.
getdave Nov 21, 2019
d9287c5
Improves handling of API request and adds comments
getdave Nov 21, 2019
55bf64c
Fix errors from conflict solving.
tellthemachines Mar 24, 2020
11a4ff8
lint
tellthemachines Mar 25, 2020
8bbe13a
Extracted fetch hook
tellthemachines Mar 25, 2020
90f92fa
Added jsdoc
tellthemachines Mar 25, 2020
dab1a15
Change API pattern.
tellthemachines Mar 31, 2020
0a5d67f
Clean up booleans and conditions
tellthemachines Mar 31, 2020
5b89402
Move hook to api-fetch package
tellthemachines Mar 31, 2020
585a6db
Attach hook to default export
tellthemachines Mar 31, 2020
641ba9e
Reset state before fetching, don't guess data type
tellthemachines Mar 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/api-fetch/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
Expand Down
35 changes: 35 additions & 0 deletions packages/api-fetch/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useEffect, useState } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -156,6 +157,38 @@ function apiFetch( options ) {
} );
}

/**
* Function that fetches data using apiFetch, and updates the status.
*
* @param {string} path Query path.
*/
function useApiFetch( path ) {
Copy link
Member

Choose a reason for hiding this comment

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

By naming standards, this should be useAPIFetch.

See: https://developer.wordpress.org/coding-standards/wordpress-coding-standards/javascript/#abbreviations-and-acronyms

// Indicate the fetching status
const [ isLoading, setIsLoading ] = useState( true );
const [ data, setData ] = useState( [] );
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed the data is referred to as dataList later on as well. The data returned from an API might also be an object, like when fetching an individual post. I think this could just be kept as an undefined initial value useState().

const [ error, setError ] = useState( null );

useEffect( () => {
apiFetch( { path } )
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be wrong, but I'm wondering if the state should be reset prior to calling apiFetch by calling setIsLoading, setData and setError again with the initial values, as I think if the path changes, useState won't override any existing values being stored.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh good point, it probably won't because only the effect will re-run.

.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,
};
}

apiFetch.use = registerMiddleware;
apiFetch.setFetchHandler = setFetchHandler;

Expand All @@ -165,4 +198,6 @@ apiFetch.createRootURLMiddleware = createRootURLMiddleware;
apiFetch.fetchAllMiddleware = fetchAllMiddleware;
apiFetch.mediaUploadMiddleware = mediaUploadMiddleware;

apiFetch.useApiFetch = useApiFetch;

export default apiFetch;
59 changes: 27 additions & 32 deletions packages/block-library/src/navigation/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ import {
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
*/
Expand All @@ -46,9 +47,6 @@ function Navigation( {
clientId,
fontSize,
hasExistingNavItems,
hasResolvedPages,
isRequestingPages,
pages,
setAttributes,
setFontSize,
updateNavItemBlocks,
Expand All @@ -57,10 +55,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 */
Copy link
Member

Choose a reason for hiding this comment

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

Do we find ourselves having to disable this often in components? If it's not a useful rule then we should just remove it...

(Not relevant to this PR!)

Copy link
Member

Choose a reason for hiding this comment

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

The value of the rule is explained here:

https://github.com/WordPress/gutenberg/blob/master/packages/eslint-plugin/docs/rules/no-unused-vars-before-return.md

Based on specific instances, we can choose to improve it as well. For example, it should probably be exempting hooks as a special circumstances, due to constraints imposed on us externally via Rules of Hooks.

Copy link
Member

Choose a reason for hiding this comment

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

The value of the rule is explained here:

I should have also mentioned there was previously quite a few offending cases that were cleaned up as part of the introduction of the rule, in case that's a more convincing method for interpreting the value.

See: #12827

Copy link
Member

Choose a reason for hiding this comment

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

It might not have been necessary to disable the rule here anyways? It was removed in #20884. If I recall correctly, the rule makes exceptions for object destructuring.

To the general problem:

we can choose to improve it as well. For example, it should probably be exempting hooks as a special circumstances, due to constraints imposed on us externally via Rules of Hooks.

This already exists as of #16737:

gutenberg/packages/eslint-plugin/configs/react.js

Lines 10 to 15 in 1f00c74

'@wordpress/no-unused-vars-before-return': [
'error',
{
excludePattern: '^use',
},
],

The problem here is that the hook is named as experimental, __experimentalUseColors. There are two ways we could accommodate for this:

  1. We can adopt a convention to reference the slot by its non-experimental name in the implementation of the component, renaming via the import (example)
  2. We can expand the pattern above to include __experimental and __unstable naming conventions

const {
TextColor,
BackgroundColor,
Expand All @@ -86,15 +84,37 @@ function Navigation( {
},
[ fontSize.size ]
);

/* eslint-enable @wordpress/no-unused-vars-before-return */

const { navigatorToolbarButton, navigatorModal } = useBlockNavigator(
clientId
);

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 );

const { isLoading: isRequestingPages, data: pages } = useApiFetch(
queryPath
);

const hasPages = !! pages && !! pages.length;

// Builds navigation links from default Pages.
const defaultPagesNavigationItems = useMemo( () => {
if ( ! pages ) {
if ( ! hasPages ) {
return null;
}

Expand Down Expand Up @@ -134,8 +154,6 @@ function Navigation( {
selectBlock( clientId );
}

const hasPages = hasResolvedPages && pages && pages.length;

const blockClassNames = classnames( className, {
[ `items-justified-${ attributes.itemsJustification }` ]: attributes.itemsJustification,
[ fontSize.class ]: fontSize.class,
Expand Down Expand Up @@ -290,31 +308,8 @@ export default compose( [
withSelect( ( select, { clientId } ) => {
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 } ) => {
Expand Down