-
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
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
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.
hmm yeah it's a little strong. I think after we re-structure the getting started page to include this it'll be unnecessary 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.
sounds good, for now I've updated the copy to be Without these optimizations, the generated CSS may contain duplicate rules that affect bundle size and debugging as mentioned in another comment.
| ## 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
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 can't really talk about it in the docs until we do, so probably best to remove this section for now
…ack to the visual
| 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
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'll see about putting this into a follow up after some of the other docs work.
| */ | ||
|
|
||
| // 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! 🎉 |
|
Build successful! 🎉 |
| 'aria-label'?: string | ||
| } | ||
|
|
||
| export function GoUpOneLink(props: GoUpOneLinkProps = {}) { |
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.
thought i deleted this in favour of the more breadcrumb looking thing?
|
Build successful! 🎉 |
cf728e8
59a0fef to
cf728e8
Compare
|
Build successful! 🎉 |
devongovett
left a comment
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.
Good for now, but I still think we might want to refactor the order of the sections a bit. I'll think about it.
| export const bannerBackground = () => 'blue-1000' as const; | ||
|
|
||
| // component.tsx | ||
| import {bannerBackground} from './style-utils' with {type: '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.
Kinda confusing IMO. You also showed custom utilities in a separate code block below. Do we need this one too?
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.
haha, this was the one that came up in slack, happy to get rid of it though, we can re-add if people run into it again
|
|
||
| ## Reusing styles | ||
|
|
||
| Extract common styles into constants and spread them into `style` calls. These must be in the same file or imported from another file as a 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.
| Extract common styles into constants and spread them into `style` calls. These must be in the same file or imported from another file as a macro. | |
| Extract common styles into constants and spread them into `style` calls within the same file. |
| }); | ||
| ``` | ||
|
|
||
| Create custom utilities by defining your own macros. |
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.
| Create custom utilities by defining your own macros as functions in a separate file. |
| </ListBoxItem> | ||
| ``` | ||
|
|
||
| ## 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.
Seems a little out of place here. External folks might be confused to see this here especially since we aren't really pushing style macros externally yet. Do you think this should be under RAC or should we have a section on the RSP side that shows how to use style macros with RAC?
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 fair point, I'm fine moving this section to the S2 styling docs side
| <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.
hmm yeah it's a little strong. I think after we re-structure the getting started page to include this it'll be unnecessary here.
| ## CSS optimization | ||
|
|
||
| The style macro relies on CSS bundling and minification for optimal output. Follow these best practices: | ||
| The `style` macro relies on CSS bundling and minification for optimal output. 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.
It's a bit strong. I wouldn't say obtuse styling bugs – not sure which bugs you mean. Maybe "Without these optimizations, the generated CSS may contain duplicate rules that affect bundle size and debugging."
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 was mainly thinking along the lines of the issues that micro frontends ran into but I guess that was resolved with the "versioning" we added to the generated css. I'll update the copy here
| ``` | ||
| ``` | ||
|
|
||
| ## Developing with style macros |
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 just "Developer tools"?
| ## Developing with style macros | ||
|
|
||
| Since `style` macros are quite different from using `className`/`style` directly, many may find it initially challenging to debug and develop against. | ||
| Below are some useful tools that may benefit your developer experience: |
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 don't think we need to say this. We don't want to scare people.
"These tools improve the developer experience when using style macros:"
| - If the viewport is larger than `2560px`, as specified by a user defined [media query](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_media_queries/Using_media_queries), the padding of the `div` is set to `64px`. | ||
| - If the viewport matches the `style` macro's predefined `lg` [breakpoint](./reference.html#conditions) (i.e. the viewport is larger than `1024px`), but does not exceed previous condition, the padding of the `div` is set to `32px` | ||
| - Otherwise, default to a padding of `8px`. |
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.
| - If the viewport is larger than `2560px`, as specified by a user defined [media query](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_media_queries/Using_media_queries), the padding of the `div` is set to `64px`. | |
| - If the viewport matches the `style` macro's predefined `lg` [breakpoint](./reference.html#conditions) (i.e. the viewport is larger than `1024px`), but does not exceed previous condition, the padding of the `div` is set to `32px` | |
| - Otherwise, default to a padding of `8px`. | |
| - If the viewport is larger than `2560px`, the padding is `64px`. | |
| - Else if the viewport matches the `lg` [breakpoint](./reference.html#conditions) (`1024px`), the padding is `32px`. | |
| - Otherwise, the padding is `8px`. |
|
|
||
| ## Conditional styles | ||
|
|
||
| Define conditional values as objects to handle media queries, UI states (hover/press), and variants. This keeps all values for a property together. |
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.
| Define conditional values as objects to handle media queries, UI states (hover/press), and variants. This keeps all values for a property together. | |
| Define conditional values such as media queries, UI states (e.g. hover, press), and style variants as objects. Conditional values are mutually exclusive: the last matching condition always wins. |
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