Skip to content

Conversation

@LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Nov 18, 2025

✅ 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:

Go to the Style Macro reference page via https://reactspectrum.blob.core.windows.net/reactspectrum/87d20590345a208e4837e8a63277a2821a764d5f/s2-docs/s2/style-macro.html and make sure the values for each of the properties still display. Feedback on experience welcome as well, especially for the updated Colors, Dimensions, and Text sections

🧢 Your Project:

RSP

@rspbot
Copy link

rspbot commented Nov 18, 2025

@LFDanLu LFDanLu marked this pull request as ready for review November 18, 2025 18:07
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.

might be nice if the first disclosure is default open within each sub level

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.

A little repeated?
Image

@LFDanLu
Copy link
Member Author

LFDanLu commented Nov 18, 2025

@snowystinger yep, what do we think about that case? I opted to keep as is since it is consistent with the following:
image

aka the full value mapping is listed and then the definition of each non-standard value follows below. I could special case these ones perhaps but would it look weird if it some were missing that "value" heading but others were?

@snowystinger
Copy link
Member

I just don't know what the first baseSpacing vs the second baseSpacing is, why is it there twice? it don't see that in the minHeight screen shot you're showing?

@LFDanLu
Copy link
Member Author

LFDanLu commented Nov 18, 2025

@snowystinger so the layout is roughly this:

Property name (e.g. minHeight)

Values that the property accepts mapped out (e.g. 'auto' | 'full' | 'min' | 'max' | 'fit' | 'screen' | number | LengthPercentage)

A set of definitions for any non-standard mappings (e.g. number/LengthPercentage)

In my screenshot, number and lengthPercentage is listed twice, once each in the "Values" section and then again below, mirroring how it appears in your screen shot of scrollPaddingBottom with baseSpacing.

which of these would be preferable?
image

OR---------------------

image

or the original?

Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

Looks much better!

@snowystinger
Copy link
Member

I think I prefer the second one, so it still says Values, but when there's only one, it omits the need to repeat it for the definition

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants