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

fix: css properties with undefined custom property values #5941

Merged
merged 3 commits into from
Mar 1, 2024

Conversation

jdawber
Copy link
Contributor

@jdawber jdawber commented Feb 25, 2024

Fixes an issue with responsive props using CSS properties with undefined custom property values.

I discovered an issue whilst developing a responsive application that when the base property is not set and the matched breakpoint is base, an undefined CSS variable is being rendered.

For example:

<View   
  backgroundColor="red-500"
  width={{
    L: "50%",
  }}
>
  Content
</View>

On a small resolution, would have a CSS result of: width: var(--spectrum-global-dimension-undefined, var(--spectrum-alias-undefined))

I'd expect width not to be set and to adhere to the default behaviour of the element type. In this case div, a block level element has width 100% within its parent container.

On a resolution matching L, the CSS result is: width: 50%

I believe this to be an issue when using responsive props for the following style handlers:

  • dimensionValue
  • backgroundColorValue
  • borderColorValue
  • borderRadiusValue

Closes: #5942

This PR updates these style handlers to return undefined instead of returning a custom property with undefined.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Change the width prop on existing default View story to be responsive without setting a base property, eg:

export const Default = args => (
  <View
    {...args}
    width={{
      L: '50%'
    }}
    height="size-500"
    elementType="span" />
);

Start storybook and view the story on a narrow window, a width property should not be set. If the window width is increased to active the L breakpoint, the width will be set to 50%.

🧢 Your Project:

@jdawber jdawber marked this pull request as ready for review February 25, 2024 16:56
@jdawber jdawber closed this Feb 25, 2024
@jdawber jdawber reopened this Feb 25, 2024
@snowystinger
Copy link
Member

GET_BUILD

@@ -206,6 +222,11 @@ export function convertStyleProps<C extends ColorVersion>(props: ViewStyleProps<

let prop = getResponsiveProp(props[key], matchedBreakpoints);
let value = convert(prop, props.colorVersion);

if (!value) {
Copy link
Member

Choose a reason for hiding this comment

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

this check is true for a few too many "falsy" values
https://www.chromatic.com/test?appId=5f0dd5ad2b5fc10022a2e320&id=65dbaab40075a76357e68393

it's gotten rid of a flexBasis={0} that it shouldn't have

<Text flexGrow={1} flexBasis={0}>Column number one. Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.</Text>

Copy link
Contributor Author

@jdawber jdawber Feb 26, 2024

Choose a reason for hiding this comment

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

This statement isn't necessary you're right it's breaking properties using style handlers such as passthroughStyle, anyValue, etc. I have removed it.

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Thanks for the update, passes VRT now. Other values look correct.

@yihuiliao yihuiliao merged commit a9399fe into adobe:main Mar 1, 2024
27 checks passed
@jdawber jdawber deleted the fix-undefined-css-var branch March 5, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid CSS property when using a Responsive prop and not setting a base value.
3 participants