-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: Style macro devtool #8305
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?
Conversation
# Conflicts: # yarn.lock
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
994911b to
2706cd1
Compare
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
# Conflicts: # package.json # packages/@react-spectrum/s2/style/__tests__/style-macro.test.js # packages/@react-spectrum/s2/style/style-macro.ts # packages/dev/parcel-config-storybook/package.json # packages/dev/s2-icon-builder/package.json # yarn.lock
|
Build successful! 🎉 |
|
Build successful! 🎉 |
differentiate between hash conflicts when same style is used in multiple files
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
reidbarber
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.
Nice! Tested in our storybook.
yihuiliao
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.
tested on the s2 storybook, i like the logo haha
| // the defaults from the style definition are omitted. | ||
| let allowedOverridesSet = new Set<string>(); | ||
| let js = 'let rules = " ";\n'; | ||
| let js = 'let rules = " ", currentRules = {};\n'; |
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.
only needed in dev builds right?
| this.pseudos = ''; | ||
| this.property = property; | ||
| this.value = value; | ||
| if (isCompilingDependencies !== null) { |
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.
Remind me what this is for? Why is it only when compiling dependencies?
| copy(): Rule | ||
| } | ||
|
|
||
| let conditionStack: string[] = []; |
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.
Perhaps at some point we can refactor this to not rely on global state
| } | ||
| if (conditionStack.length) { | ||
| // name += ` (${conditionStack.join(', ')})`; | ||
| res += ` currentRules[${JSON.stringify(name)}] = typeof currentRules[${JSON.stringify(name)}] === 'object' ? currentRules[${JSON.stringify(name)}] : {"default": currentRules[${JSON.stringify(name)}]}; currentRules[${JSON.stringify(name)}][${JSON.stringify(conditionStack.join(' && '))}] = ${JSON.stringify(this.themeValue)};`; |
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 is kinda hard to read. A comment would help. Also maybe store JSON.stringify(name) in a variable and use that?
| @@ -0,0 +1,384 @@ | |||
| # style-macro-chrome-plugin | |||
|
|
|||
| This is a chrome plugin to assist in debugging the styles applied by our 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.
| This is a chrome plugin to assist in debugging the styles applied by our Style Macro. | |
| This is a chrome plugin to assist in debugging the styles applied by the React Spectrum Style Macro. |
| - DevTools reads directly from CSS via `getComputedStyle()` | ||
|
|
||
| **Dynamic Macros** (`-macro-dynamic-{hash}`): | ||
| - Used when style conditions can change (e.g., `style({ color: isActive ? 'red' : 'blue' })`) |
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.
| - Used when style conditions can change (e.g., `style({ color: isActive ? 'red' : 'blue' })`) | |
| - Used when style conditions can change (e.g., `style({color: {default: 'blue', isActive: 'red'}})`) |
| | `macro-response` | Content → Background → DevTools | Return requested macro data | | ||
| | `get-macro` | Background → Content | Internal forwarding of query-macros | | ||
| | `init` | DevTools → Background | Establish connection with tabId | | ||
| | `class-changed` | Page → Content → Background → DevTools | Notify that selected element's className changed | |
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 might want to namespace these in case other scripts/extensions have the same names
| chrome.runtime.onMessage.addListener((message, _sender, _sendResponse) => { | ||
| debugLog('Received message:', message); | ||
|
|
||
| if (message.action === 'get-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.
Does it make sense to store the macro data directly in the dev tools instead of in the content script? i.e. have the content script send the macro data in the update-macros message instead of storing in window.__macros and then requesting it asynchronously via get-macro?
| // Generate a unique ID if element doesn't have one | ||
| if (!element.hasAttribute('data-devtools-id')) { | ||
| element.setAttribute('data-devtools-id', 'dt-' + Date.now() + '-' + Math.random().toString(36).substr(2, 9)); |
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 use a property instead of an attribute so it is more hidden and less likely to get overwritten by something?
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.
😂
Closes
Follow instructions in README to run the plugin. There is also a description of the extension architecture in there as it is non-trivial due to the various sandbox that chrome imposes in addition to the different ways that a macro could be updated ie. static, dynamic, conditional static.
Note, there is a parcel bug which will cause all tabs that have had the content script injected to reload on every change. I cannot resolve this right now. If you build instead so that the extension doesn't change, then it won't do this. It's only during HMR. This will also cause every tab to refresh if a tab that had loaded the extension is closed.
For this reason, it can be easier to build the extension instead of running it as if you're developing.
Some notes, this is currently meant to be used in addition to the regular panels and AtomicCSS to determine where styles are coming from. It is not designed to replace those extensions and inspectors.
✅ Pull Request Checklist:
📝 Test Instructions:
I added a style macro in the commit e5ab27b which is static and is swapped with another static one on a span inside buttons. It reacts to the button being focused. You can see the extensions dev panel is updated when this swap occurs despite it being a static macro.
I removed that afterwards. So to test the updates on static macros, use that storybook build.
Otherwise, you can inspect elements and view the results in the dev tools panel.
🧢 Your Project: