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

CustomSelect: adjust renderSelectedValue to fix sizing #57865

Merged
merged 4 commits into from
Jan 18, 2024

Conversation

brookewp
Copy link
Contributor

What?

When creating the component, we introduced logic to have better default values for rendered select values. However, this affected the sizing styles, which relied on the boolean hasCustomRenderProp to check if renderSelectedValue is true or not. In trunk, renderSelectedValue will always return true so the styles aren't applying as expected.

Why?

To ensure the component size can be changed via the size prop.

How?

To fix this, we can remove defaultRenderSelectedValue as the default value for renderSelectedValue and instead invoke the function only when renderSelectedValue is true.

If renderSelectedValue is false, the currentValue will be used, and if that is empty, Ariakit uses the first available value.

Testing Instructions

  1. npm run storybook:dev
  2. Go to CustomSelectControl V2 story
  3. Ensure the styles for size small are applied to the 'Default' story (they won't apply to the other stories, where hasCustomRenderProp is true.
@brookewp brookewp added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Jan 16, 2024
@brookewp brookewp requested a review from a team January 16, 2024 01:38
@brookewp brookewp self-assigned this Jan 16, 2024
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Nice find! I believe that we can achieve the same result with fewer changes, and while keeping the defaultRenderSelectedValue external to the component's render function.

We could remove the default assignment to the renderSelectedValue (as you already did in this PR), and basically move the "use the default value if no value is provided" logic in the components' render function — something like

const computedRenderSelectedValue = renderSelectedValue ?? defaultRenderSelectedValue

We could then use computedRenderSelectedValue instead of renderSelectedValue in the JSX code.

What do you think?

@ciampo ciampo added this to In progress (owned) ⏳ in WordPress Components via automation Jan 16, 2024
@brookewp
Copy link
Contributor Author

Nice find! I believe that we can achieve the same result with fewer changes, and while keeping the defaultRenderSelectedValue external to the component's render function.

We could remove the default assignment to the renderSelectedValue (as you already did in this PR), and basically move the "use the default value if no value is provided" logic in the components' render function — something like

const computedRenderSelectedValue = renderSelectedValue ?? defaultRenderSelectedValue

We could then use computedRenderSelectedValue instead of renderSelectedValue in the JSX code.

What do you think?

Sounds great! I've implemented your solution in: dc2635e

@brookewp brookewp requested review from ciampo and a team January 17, 2024 02:38
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Good to merge after adding a CHANGELOG entry :)

Copy link

github-actions bot commented Jan 18, 2024

Flaky tests detected in 96f0890.
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/7563884228
📝 Reported issues:

@ciampo ciampo enabled auto-merge (squash) January 18, 2024 06:59
@ciampo
Copy link
Contributor

ciampo commented Jan 18, 2024

There are some e2e failures (Firefox getting a 500 server error) that are not related to the changes from this PR, especially because the affect code is not used nor exported by the @wordpress/components package. Therefore, I'm going to merge this PR despite required checks not passing.

@ciampo ciampo disabled auto-merge January 18, 2024 09:18
@ciampo ciampo merged commit bc6e2e2 into trunk Jan 18, 2024
55 of 56 checks passed
WordPress Components automation moved this from In progress (owned) ⏳ to Done 🎉 Jan 18, 2024
@ciampo ciampo deleted the fix/customselect-render branch January 18, 2024 09:18
@github-actions github-actions bot added this to the Gutenberg 17.6 milestone Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
2 participants