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.

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

DataForm: provide a better default for render when field has elements #64338

Merged
merged 2 commits into from
Aug 7, 2024

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Aug 7, 2024

Related #55101
Follow-up to #64299 (comment)

What?

Provide a better default render function when the field has declared elements.

Why?

Note how the author and status fields are rendered in the before/after.

Before:

Gravacao.do.ecra.2024-08-07.as.14.19.10.mov

After:

Gravacao.do.ecra.2024-08-07.as.16.31.45.mov

Testing Instructions

npm install && npm run storybook:dev and verify author and status are properly rendered.

Copy link

github-actions bot commented Aug 7, 2024

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: oandregal <oandregal@git.wordpress.org>
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>

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

@oandregal oandregal self-assigned this Aug 7, 2024
@oandregal oandregal added [Type] Bug An existing feature does not function as intended [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond labels Aug 7, 2024
return (
field?.elements?.find( ( element ) => element.value === value )
?.label || getValue( { item } )
);
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to start adding some unit tests for this function

@oandregal
Copy link
Member Author

The fix doesn't seem to work well with author: looking into it.

@youknowriad youknowriad changed the title DatForm: provide a better default for render when field has elements DataForm: provide a better default for render when field has elements Aug 7, 2024
Copy link

github-actions bot commented Aug 7, 2024

Flaky tests detected in c82506b.
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/10286303872
📝 Reported issues:

@oandregal oandregal enabled auto-merge (squash) August 7, 2024 14:54
const label = field?.elements?.find( ( element ) => {
// Intentionally using == here to allow for type coercion.
// eslint-disable-next-line eqeqeq
return element.value == value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the issue in the field though. If the field is "string" its values should be string both in getValue and elements for me. and same for integer no?

Copy link
Member Author

Choose a reason for hiding this comment

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

#64391 may be a better approach.

@oandregal oandregal merged commit 50419e5 into trunk Aug 7, 2024
61 checks passed
@oandregal oandregal deleted the fix/dataform-panel-render-with-elements branch August 7, 2024 15:29
@github-actions github-actions bot added this to the Gutenberg 19.0 milestone Aug 7, 2024
@@ -18,7 +18,7 @@ export function normalizeFields< Item >(

const getValue =
field.getValue ||
( ( { item }: { item: ItemRecord } ) => item[ field.id ] );
( ( { item }: { item: Item } ) => item[ field.id as keyof Item ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I didn't notice this before. I see this was addressed at #64367

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, not important right now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants