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

improve documentation for fields package #67580

Merged
merged 1 commit into from
Dec 5, 2024
Merged

improve documentation for fields package #67580

merged 1 commit into from
Dec 5, 2024

Conversation

gigitux
Copy link
Contributor

@gigitux gigitux commented Dec 4, 2024 •
edited
Loading

What?

This pull request includes documentation updates for various actions and fields in the packages/fields package. Also, remove unused native files.

I think that there is still some space for improvements (especially on action names) but we can revisit later.

@gigitux gigitux added the [Type] Developer Documentation Documentation for developers label Dec 4, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 4, 2024

Flaky tests detected in f55d10c.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12161690957
📝 Reported issues:

@gigitux gigitux self-assigned this Dec 4, 2024
@gigitux gigitux added the [Package] Fields /packages/fields label Dec 4, 2024
gigitux
gigitux commented Dec 4, 2024
View reviewed changes
packages/fields/src/fields/template/index.ts
@@ -14,9 +14,11 @@ const templateField: Field< BasePost > = {
id: 'template',
type: 'text',
label: __( 'Template' ),
getValue: ( { item } ) => item.template,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not necessary. I removed it. #66591 (comment)

@gigitux gigitux marked this pull request as ready for review December 4, 2024 14:49
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 4, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: gigitux <[email protected]>
Co-authored-by: oandregal <[email protected]>
Co-authored-by: youknowriad <[email protected]>
Co-authored-by: dcalhoun <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

oandregal
oandregal reviewed Dec 4, 2024
View reviewed changes
packages/fields/src/actions/duplicate-post.native.tsx
@@ -1,3 +0,0 @@
const duplicatePost = undefined;

export default duplicatePost;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we delete this file? Is it not used by mobile (I see it's named *.native.*)?

oandregal
oandregal reviewed Dec 4, 2024
View reviewed changes
packages/fields/src/actions/duplicate-post.tsx
@@ -37,8 +37,8 @@ const duplicatePost: Action< BasePost > = {
const [ item, setItem ] = useState< BasePost >( {
...items[ 0 ],
title: sprintf(
/* translators: %s: Existing template title */
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch, thanks! We actually hide this action for templates.

oandregal
oandregal reviewed Dec 4, 2024
View reviewed changes
packages/fields/src/actions/export-pattern.native.tsx
@@ -1,3 +0,0 @@
const exportPattern = undefined;

export default exportPattern;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above: isn't this used by mobile?

@oandregal
Copy link
Member

Thanks for taking the time :)

I'd approve but I'd like to double-check why we're removing the *.native.* files used by mobile.

@gigitux
Copy link
Contributor Author

gigitux commented Dec 4, 2024

Thanks for taking the time :)

I'd approve but I'd like to double-check why we're removing the *.native.* files used by mobile.

These two actions exportsundefined. Also, if you search for exportPatternNative, duplicatePostNative and reorderPageNative, I can't find any reference:

image

image

image

Also, mobile build doesn't fail, so I think that these changes are fine.

@oandregal
Copy link
Member

Also, mobile build doesn't fail, so I think that these changes are fine.

They actually don't run :) There's an issue with the runner mobile folks are looking at #67586

oandregal
oandregal reviewed Dec 4, 2024
View reviewed changes
packages/fields/src/actions/index.ts
@@ -1,13 +1,10 @@
export { default as viewPost } from './view-post';
export { default as reorderPage } from './reorder-page';
export { default as reorderPageNative } from './reorder-page.native';
Copy link
Member

Choose a reason for hiding this comment

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

@dcalhoun sorry to ping you so many times lately :)

Is it ok to remove the native actions here? They export undefined (reorder), export pattern, duplicate post, so we wonder why are they necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly to make unit tests pass. If nothing breaks, it's ok to remove IMO

Copy link
Member

Choose a reason for hiding this comment

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

I'll concede to @youknowriad's knowledge on this one. It appears the addition occurred in 94d1d2b to repair unit test failures. I agree it is safe to remove if unit tests continue passing.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the confirmation to both of you 🙇

@oandregal
Copy link
Member

Given the native tests are blocked, I ran them locally (npm run test:native) and they pass:

Screenshot 2024-12-05 at 10 40 59

@gigitux
Copy link
Contributor Author

gigitux commented Dec 5, 2024 •
edited
Loading

Thanks for the feedback and review folks! 🙇

@gigitux gigitux merged commit 09b2897 into trunk Dec 5, 2024
79 of 85 checks passed
@gigitux gigitux deleted the improve/fields-documentation branch December 5, 2024 09:43
@github-actions github-actions bot added this to the Gutenberg 19.9 milestone Dec 5, 2024
michalczaplinski pushed a commit that referenced this pull request Dec 5, 2024
@gigitux @michalczaplinski
improve documentation for fields package (#67580)
8e2fce3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcalhoun dcalhoun dcalhoun left review comments

@oandregal oandregal oandregal approved these changes

@youknowriad youknowriad Awaiting requested review from youknowriad

@louwie17 louwie17 Awaiting requested review from louwie17

Assignees

@gigitux gigitux

Labels
[Package] Fields /packages/fields [Type] Developer Documentation Documentation for developers
Projects
None yet
Milestone
Gutenberg 19.9
Development

Successfully merging this pull request may close these issues.
4 participants
@gigitux @oandregal @youknowriad @dcalhoun