-
Notifications
You must be signed in to change notification settings - Fork 1.3k
docs: Style macro docs #9090
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
base: main
Are you sure you want to change the base?
docs: Style macro docs #9090
Conversation
|
Build successful! 🎉 |
| - The [atomic-css-devtools](https://github.com/astahmer/atomic-css-devtools) extension presents an inspected element's atomic CSS rules | ||
| in a non-atomic format, making it easier to scan. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: replace with the new devtool extension @snowystinger is making
| This indicates that your `style` macro has a condition that isn't evaluable at build time. This can happen for a variety of reasons such | ||
| as if you've referenced non-constant variables within your `style` macro or if you've called non-macro functions within your `style` macro. | ||
| If you are using Typescript, it can be as simple as forgetting to add `as const` to your own defined reusable macro. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add an example of a macro that would cause this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel like there's several ways you could encounter this error (based on vibes) so a single example wouldn't be enough and might cause confusion for some people if their code doesn't match the example.
maybe would be good to find the most common ways you can encounter this error and offer a couple of possible solutions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, its just been kinda hard finding which ways are the common ways people encounter this. I tried to allude to those fixes above but didn't want to fully document each since it might take too much room
| > I tried to pass my `style` macro to `UNSAFE_className` but it doesn't work. | ||
|
|
||
| The `style` macro is not meant to be used with `UNSAFE_className`. Overrides to the Spectrum styles is highly discouraged, | ||
| consider styling an equivalent React Aria Component instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already mentioned in the UNSAFE style overrides section but felt it was worth adding here anyways
| <InlineAlert variant="informative"> | ||
| <Heading>Important Note</Heading> | ||
| <Content> | ||
| Due to the atomic nature of the generated CSS rules, it is strongly recommended that you follow the best practices listed [below](#css-optimization). | ||
| Failure to do so can result in large number of duplicate rules and obtuse styling bugs. | ||
| </Content> | ||
| </InlineAlert> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe overkill, but it felt like it was important to mention this up front so people realize that this is important
| ## Creating custom components | ||
|
|
||
| In-depth examples are coming soon! | ||
|
|
||
| `mergeStyles` can be used to merge the style strings from multiple macros together, with the latter styles taking precedence similar to object spreading. | ||
| This behavior can be leveraged to create a component API that allows an end user to only override specific styles conditionally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed previously that for this first pass that we want to stick with just modifying this page and create examples of style macros with RAC after the release of the docs. I've opted to keep this section and the once above it for now just to let people know about the existence of mergeStyles/iconStyle/and setting CSS variables with macros, but ideally these would move into an "Advanced" page or something similar to cut down on length here. Will comment on the file as to what I'm thinking the final division should look like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we export mergeStyles yet? I don't see it in our exports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not yet, but there was mention of doing so a while back in the beta slack channel. I've put it in the S2 1.0 canvas so we can track that
…ack to the visual
| - If you are using Cursor, we offer a set of [Cursor rules](https://github.com/adobe/react-spectrum/blob/main/rules/style-macro.mdc) to use when developing with style macros. Additionally, | ||
| we have MCP servers for [React Aria](#TODO) and [React Spectrum](https://www.npmjs.com/package/@react-spectrum/mcp) respectively that interface with the docs. | ||
|
|
||
| ## FAQ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we make use of disclosures here? (but maybe a follow-up if it takes too long to style and what not)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats a good suggestion, I was a bit torn on if the current style was better or now (inspired by our own FAQ in github) but I've definitely seen both styles on other sites like MUI and Untitled UI. I'll bring this up with the rest of the team and see what people like
| This indicates that your `style` macro has a condition that isn't evaluable at build time. This can happen for a variety of reasons such | ||
| as if you've referenced non-constant variables within your `style` macro or if you've called non-macro functions within your `style` macro. | ||
| If you are using Typescript, it can be as simple as forgetting to add `as const` to your own defined reusable macro. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i feel like there's several ways you could encounter this error (based on vibes) so a single example wouldn't be enough and might cause confusion for some people if their code doesn't match the example.
maybe would be good to find the most common ways you can encounter this error and offer a couple of possible solutions
|
|
||
| ## Text | ||
|
|
||
| Spectrum 2 typography can be applied via the `font` [shorthand](#shorthand), which sets `fontFamily`, `fontSize`, `fontWeight`, `lineHeight`, and `color`. You can override any of these individually. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this repeat the content under Typography for the most part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thats right, the content here is the same. Open to opinions if it should stay like that, I felt like some people might jump to this page on its own since it is a reference and therefore it would be useful to have the additional context/explanation
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| <Heading>Important Note</Heading> | ||
| <Content> | ||
| Only use `<Heading>` and `<Text>` inside Spectrum components with predefined styles (e.g., `<Dialog>`, `<MenuItem>`). They are unstyled by default and should not be used standalone. Use HTML elements with the style macro instead. | ||
| Only use `<Heading>` and `<Text>` inside Spectrum components with predefined styles (e.g., `<Dialog>`, `<MenuItem>`). They are unstyled by default and should not be used standalone. Use HTML elements with the `style` macro instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The typography section should also explain that you're expected to specify the font everywhere, we do not set it at a global level any longer
people have gotten confused on this point
I'll send a link in slack for one of the discussions
| <Button styles={buttonStyle}>Press me</Button> | ||
| ``` | ||
|
|
||
| ## CSS optimization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optimization sounds so optional
maybe it'd be better to structure all bundler setups into their own page which includes how the optimization setup works, but then it all sounds much more mandatory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
happy for the team to weigh in here then, especially since it would fall within the changes made in #9126
| @@ -0,0 +1,217 @@ | |||
| import {Layout} from '../../../src/Layout'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't seem to visit this page in the build yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this page being advanced.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does clicking on the "Advanced" in the ToC work for you here? https://reactspectrum.blob.core.windows.net/reactspectrum/30bab2283c4022fe8d4570468b80963ddbd4a741/s2-docs/s2/styling.html
Brings me here: https://reactspectrum.blob.core.windows.net/reactspectrum/30bab2283c4022fe8d4570468b80963ddbd4a741/s2-docs/s2/styling/advanced.html which seems to work
| import React from 'react'; | ||
| import {style} from '@react-spectrum/s2/style' with {type: 'macro'}; | ||
|
|
||
| export function S2FAQ() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably pass Disclosure and DisclosurePanel to components for summary/details so we don't have to create separate components for each accordion we want
Otherwise, i suggest creating a single component that accepts the title and panel contents so we can keep the content inside the pages and have a reusable accordion outside in the components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could probably pass Disclosure and DisclosurePanel to components for summary/details so we don't have to create separate components for each accordion we want
not quite sure what you mean here, mind expanding a bit on components here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
components is a map that's passed to the mdx renderer in Layout, it matches tags as the document is rendered and applies whatever rendering is in the map for that tag, so we should be able to match summary/description in that map and render our own Disclosure for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might need to chat to you directly, still not quite envisioning what the change here would be, but I think you are suggesting that in the mdx we'd put <Disclosure> and DisclosurePanel> directly and that Layout could then handle rendering that?
i.e.
styling.mdx
<Disclosure>
...FAQ content
</Disclosure>
happy to do that, but honestly I almost prefer the separate component if only for type checking in the IDE haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i guess our component is a little too complicated to just represent with details/summary
but i vote for putting it into the MDX because this is all content, and content really shouldn't be in src
| */ | ||
|
|
||
| // Properties that use PercentageProperty (accept LengthPercentage in addition to mapped values) | ||
| const percentageProperties = new Set([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this file inside the s2 pages area, it's a ts file, so won't be picked up as a new page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to note that it is used by types.tsx in the same folder hence why I kept it here. Didn't really feel like it fit in the pages area since that is where the mdx pages were and we already had things like S2Colors/etc here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should make a bit of a push to move content out of the components, but fine if there's others for now
| the `style` macros for quick prototyping. | ||
|
|
||
| - If you are using Cursor, we offer a set of [Cursor rules](https://github.com/adobe/react-spectrum/blob/main/rules/style-macro.mdc) to use when developing with style macros. Additionally, | ||
| we have MCP servers for [React Aria](#TODO) and [React Spectrum](https://www.npmjs.com/package/@react-spectrum/mcp) respectively that interface with the docs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can link to the respective /mcp docs pages instead.
|
Build successful! 🎉 |
|
Build successful! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving for testing, just a few comments.
|
|
||
| export const omitFromNav = true; | ||
| export const tags = ['style', 'macro', 'spectrum', 'custom', 'values', 'reference']; | ||
| export const description = 'Reference table for the `style` macro'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <S2Colors /> | ||
|
|
||
| <StyleMacroProperties properties={getPropertyDefinitions('color')} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <MyComponent styles={userStyles} /> | ||
| ``` | ||
|
|
||
| The `iconStyle` macro should be used when styling Icons, see the [docs](../icons.html#iconstyle) for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This link is currently broken on the build because it needs be capitalized (../Icons.html#iconstyle), but I actually think it's correct to lowercase this page since it isn't a component. We could update this in a follow-up (we'd probably need to update a bunch of links).
|
I got a 404 when trying to click on the "related pages" links |
|
@devongovett the related pages links on https://reactspectrum.blob.core.windows.net/reactspectrum/30bab2283c4022fe8d4570468b80963ddbd4a741/s2-docs/s2/styling.html? Those seem to work for me |
|
Build successful! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For those reviewing this page, compare the current experience with the old: https://reactspectrum.blob.core.windows.net/reactspectrum/5084b6bfa7066fbe90e520c9575422bf0df27dd0/s2-docs/s2/styling.html
In particular the organization/existence of subpage/contents, open to opinions as to which is preferred
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| {id: 'DropZone', name: 'DropZone', href: 'DropZone.html'}, | ||
| {id: 'Form', name: 'Form', href: 'Form.html'}, | ||
| {id: 'Icons', name: 'Icons', href: 'Icons.html'}, | ||
| {id: 'Icons', name: 'Icons', href: 'icons.html'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would this be the only one with a lowercase? seems like the file should be renamed if that's what it relies on, that way it's more predictable.
really i'd like to get rid of all the uppercases in the urls... but that's a different issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the convention that seems to exist currently is that component pages are capitalized (e.g. DropZone/Menu/etc) and that non component pages aren't (e.g. dnd.mdx/migrating.mdx/etc) and icons fits into the latter. The file should be renamed to be lower case as well with my last commit, forgot to force git to recognize that change since it didn't automatically
| ## Creating custom components | ||
|
|
||
| In-depth examples are coming soon! | ||
|
|
||
| `mergeStyles` can be used to merge the style strings from multiple macros together, with the latter styles taking precedence similar to object spreading. | ||
| This behavior can be leveraged to create a component API that allows an end user to only override specific styles conditionally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we export mergeStyles yet? I don't see it in our exports?
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| - Use a CSS minifier like `lightningcss` to deduplicate common rules (consider in dev for easier debugging). | ||
| - Bundle all CSS for S2 components and style macros into a single CSS bundle rather than code splitting to avoid duplicate rules across chunks. | ||
|
|
||
| **Failure to perform this optimization will result in a suboptimal developer experience and obtuse styling bugs.** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reuse that inline alert you wrote?
| import React from 'react'; | ||
| import {style} from '@react-spectrum/s2/style' with {type: 'macro'}; | ||
|
|
||
| export function S2FAQ() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i guess our component is a little too complicated to just represent with details/summary
but i vote for putting it into the MDX because this is all content, and content really shouldn't be in src


Closes
✅ Pull Request Checklist:
📝 Test Instructions:
Go to the S2 Styling page and check the content. Also make sure the "Advanced" and "Reference Table" pages show up in desktop and mobile and check the content there as well
🧢 Your Project:
RSP