Revert changes to Nav Block data fetching mechanics #21721

getdave · 2020-04-20T08:53:00Z

This PR has been extracted from #21674.

It partially reverts commit f2b6778 (based on PR #18669) which changed the data fetching mechanics for the Navigation Block to use apiFetch (via a new useApiFetch hook).

This PR only reverts the changes to the Nav Block to utilise useApiFetch Hook. The deprecation of useApiFetch will be handled in a separate PR.

Note this does not solve the issue where Contributor users cannot "Create from top-level Pages" because of the context parameter being force set to edit within the @wordpress/core-data package. If this were overridable and could be set to view then this issue would be fixed, but that should be addressed in another PR via a change to core-data (cc @nerrad ).

Description

Reverted the Nav Block data fetch mechanics back to how it was prior to #18669.

How has this been tested?

Check e2e tests still pass.
Check Block behaves correctly and without errors in the Editor.

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.
I've updated all React Native files affected by any refactorings/renamings in this PR.

…loadPostTypeEntities` (#18669)" This partially reverts commit f2b6778. Only reverts the changes to utilise useApiFetch Hook.

github-actions · 2020-04-20T08:59:07Z

Size Change: +81 B (0%)

Total Size: 842 kB

Filename	Size	Change
`build/annotations/index.js`	3.62 kB	-1 B
`build/block-directory/index.js`	6.24 kB	-1 B
`build/block-editor/index.js`	105 kB	+13 B (0%)
`build/block-library/index.js`	112 kB	+68 B (0%)
`build/blocks/index.js`	57.7 kB	-1 B
`build/data-controls/index.js`	1.25 kB	+1 B
`build/edit-post/index.js`	27.9 kB	+2 B (0%)
`build/editor/index.js`	43.3 kB	+1 B
`build/keyboard-shortcuts/index.js`	2.51 kB	-1 B
`build/list-reusable-blocks/index.js`	3.12 kB	-1 B
`build/media-utils/index.js`	5.29 kB	-1 B
`build/server-side-render/index.js`	2.67 kB	+2 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.02 kB	0 B
`build/api-fetch/index.js`	4.01 kB	0 B
`build/autop/index.js`	2.82 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/style-rtl.css`	760 B	0 B
`build/block-directory/style.css`	761 B	0 B
`build/block-editor/style-rtl.css`	10.2 kB	0 B
`build/block-editor/style.css`	10.2 kB	0 B
`build/block-library/editor-rtl.css`	7.1 kB	0 B
`build/block-library/editor.css`	7.1 kB	0 B
`build/block-library/style-rtl.css`	7.17 kB	0 B
`build/block-library/style.css`	7.17 kB	0 B
`build/block-library/theme-rtl.css`	683 B	0 B
`build/block-library/theme.css`	685 B	0 B
`build/block-serialization-default-parser/index.js`	1.88 kB	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/components/index.js`	198 kB	0 B
`build/components/style-rtl.css`	16.9 kB	0 B
`build/components/style.css`	16.9 kB	0 B
`build/compose/index.js`	6.66 kB	0 B
`build/core-data/index.js`	11.4 kB	0 B
`build/data/index.js`	8.43 kB	0 B
`build/date/index.js`	5.47 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	569 B	0 B
`build/dom/index.js`	3.1 kB	0 B
`build/edit-navigation/index.js`	3.54 kB	0 B
`build/edit-navigation/style-rtl.css`	485 B	0 B
`build/edit-navigation/style.css`	485 B	0 B
`build/edit-post/style-rtl.css`	12.3 kB	0 B
`build/edit-post/style.css`	12.3 kB	0 B
`build/edit-site/index.js`	10.5 kB	0 B
`build/edit-site/style-rtl.css`	5.04 kB	0 B
`build/edit-site/style.css`	5.04 kB	0 B
`build/edit-widgets/index.js`	7.49 kB	0 B
`build/edit-widgets/style-rtl.css`	4.66 kB	0 B
`build/edit-widgets/style.css`	4.66 kB	0 B
`build/editor/editor-styles-rtl.css`	428 B	0 B
`build/editor/editor-styles.css`	431 B	0 B
`build/editor/style-rtl.css`	3.48 kB	0 B
`build/editor/style.css`	3.47 kB	0 B
`build/element/index.js`	4.65 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/index.js`	7.32 kB	0 B
`build/format-library/style-rtl.css`	502 B	0 B
`build/format-library/style.css`	502 B	0 B
`build/hooks/index.js`	2.13 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/i18n/index.js`	3.56 kB	0 B
`build/is-shallow-equal/index.js`	711 B	0 B
`build/keycodes/index.js`	1.94 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	226 B	0 B
`build/list-reusable-blocks/style.css`	226 B	0 B
`build/notices/index.js`	1.79 kB	0 B
`build/nux/index.js`	3.4 kB	0 B
`build/nux/style-rtl.css`	616 B	0 B
`build/nux/style.css`	613 B	0 B
`build/plugins/index.js`	2.67 kB	0 B
`build/primitives/index.js`	1.49 kB	0 B
`build/priority-queue/index.js`	789 B	0 B
`build/redux-routine/index.js`	2.84 kB	0 B
`build/rich-text/index.js`	14.8 kB	0 B
`build/shortcode/index.js`	1.7 kB	0 B
`build/token-list/index.js`	1.28 kB	0 B
`build/url/index.js`	4.02 kB	0 B
`build/viewport/index.js`	1.84 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

packages/block-library/src/navigation/edit.js

marekhrabe

I have manually tested the functionality around the "Create from all top-level pages" button and that has no regressions and still works well. Can you confirm that's what I was supposed to check here? Otherwise, this seems all good.

I've left two code-style questions/comments

getdave · 2020-04-20T12:33:21Z

I have manually tested the functionality around the "Create from all top-level pages" button and that has no regressions and still works well.

Much appreciated @marekhrabe 🎉🎉🎉🎉🎉

Can you confirm that's what I was supposed to check here? Otherwise, this seems all good.

Yeh basically:

Does Create from Top Level Pages work?
Does creating empty work?
Can you add / edit / remove Nav Items?

I've left two code-style questions/comments

Resolve one and the other I'd prefer to handle elsewhere. Let me know if we're still 👍

marekhrabe

All working well. Thanks for addressing the feedback!

chrisvanpatten · 2020-04-20T13:54:55Z

Does this still come with the caveat about contributor users?

getdave · 2020-04-20T14:18:40Z

Does this still come with the caveat about contributor users?

Yes sorry it does. I've updated the PR desc to make this clear.

I checked and it's still not possible to overide the context param on the getEntityRecords resolver in @wordpress/core-data therefore something like this will not work:

const filterDefaultPages = {
    parent: 0,
    order: 'asc',
    orderby: 'id',
    context: 'view', // note this will be overridden at https://github.com/WordPress/gutenberg/blob/1c7d80b7ad8025b2cead7866e49103911cf4889a/packages/core-data/src/resolvers.js#L97
};

const pagesSelect = [
    'core',
    'getEntityRecords',
    [ 'postType', 'page', filterDefaultPages ],
];

I suggest we reopen the original issue and then tackle in a new PR.

chrisvanpatten · 2020-04-20T17:49:36Z

@getdave It may also be worth opening a more general ticket to discuss making it possible to override context in any situation, as an edit context should not always be required (sometimes you may want to get an entity record for viewing, for instance). But I'll wait to see what the outcome of the discussion is on #18659 before doing that.

aduth

I haven't tested it, but it looks like a valid partial revert of f2b6778.

Partially Revert "Fix Nav Block bug as Contributor user via fix to `*…

e2f83f2

…loadPostTypeEntities` (#18669)" This partially reverts commit f2b6778. Only reverts the changes to utilise useApiFetch Hook.

getdave added the [Block] Navigation Affects the Navigation Block label Apr 20, 2020

getdave requested review from retrofox, marekhrabe, youknowriad, noisysocks and aduth April 20, 2020 08:53

getdave marked this pull request as ready for review April 20, 2020 08:53

getdave requested review from ajitbohra, jorgefilipecosta, Soean and talldan as code owners April 20, 2020 08:53

getdave mentioned this pull request Apr 20, 2020

Revert introduction of useApiFetch hook and associated changes to Core Navigation block data fetching. #21674

Closed

6 tasks

This was referenced Apr 20, 2020

Adds deprecation notice to useApiFetch hook #21723

Merged

Nav block: enable creation from existing WP Menus #18869

Merged

marekhrabe reviewed Apr 20, 2020

View reviewed changes

packages/block-library/src/navigation/edit.js Show resolved Hide resolved

marekhrabe reviewed Apr 20, 2020

View reviewed changes

packages/block-library/src/navigation/edit.js Outdated Show resolved Hide resolved

marekhrabe reviewed Apr 20, 2020

View reviewed changes

Remove unneeded eslint disable rule

7c3bb6d

marekhrabe approved these changes Apr 20, 2020

View reviewed changes

getdave self-assigned this Apr 21, 2020

aduth approved these changes Apr 21, 2020

View reviewed changes

getdave merged commit 8885eec into master Apr 21, 2020

getdave deleted the fix/revert-nav-block-data-fetch-changes branch April 21, 2020 20:45

github-actions bot added this to the Gutenberg 8.0 milestone Apr 21, 2020

chrisvanpatten mentioned this pull request Apr 21, 2020

core/navigation still uses useApiFetch #21771

Closed

getdave mentioned this pull request Apr 22, 2020

Fix e2e tests by removing unecessary assertions & properly installing deps in api-fetch package #21780

Merged

6 tasks

aduth mentioned this pull request May 13, 2020

API Fetch: Remove deprecated useApiFetch #22333

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert changes to Nav Block data fetching mechanics #21721

Revert changes to Nav Block data fetching mechanics #21721

getdave commented Apr 20, 2020 •

edited

github-actions bot commented Apr 20, 2020 •

edited

marekhrabe left a comment

getdave commented Apr 20, 2020

marekhrabe left a comment

chrisvanpatten commented Apr 20, 2020

getdave commented Apr 20, 2020 •

edited

chrisvanpatten commented Apr 20, 2020

aduth left a comment

Revert changes to Nav Block data fetching mechanics #21721

Revert changes to Nav Block data fetching mechanics #21721

Conversation

getdave commented Apr 20, 2020 • edited

Description

How has this been tested?

Types of changes

Checklist:

github-actions bot commented Apr 20, 2020 • edited

marekhrabe left a comment

Choose a reason for hiding this comment

getdave commented Apr 20, 2020

marekhrabe left a comment

Choose a reason for hiding this comment

chrisvanpatten commented Apr 20, 2020

getdave commented Apr 20, 2020 • edited

chrisvanpatten commented Apr 20, 2020

aduth left a comment

Choose a reason for hiding this comment

getdave commented Apr 20, 2020 •

edited

github-actions bot commented Apr 20, 2020 •

edited

getdave commented Apr 20, 2020 •

edited