feat(link|button): support global button styling for native and slotted elements#6051
feat(link|button): support global button styling for native and slotted elements#6051
Conversation
🦋 Changeset detectedLatest commit: 19b5219 The changes in this PR will be included in the next version bump. This PR includes changesets to release 83 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
5t3ph
added
a11y
| import { TemplateResult } from '@spectrum-web-components/base'; | ||
|
|
||
| import { Properties, renderButtonSet } from './index.js'; | ||
| import { makeOverBackground, Properties, renderButtonSet } from './index.js'; |
There was a problem hiding this comment.
All of these legacy story changes have to do with correctly supporting the display of static colors
📚 Branch Preview Links🔍 First Generation Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
2nd-gen/packages/swc/.storybook/guides/customization/global-elements.mdx
Show resolved
Hide resolved
2nd-gen/packages/swc/.storybook/guides/customization/global-elements.mdx
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| /* Results in encapsulating styles, preventing stronger application selectors from overriding basic properties like `color` */ |
There was a problem hiding this comment.
Noted also in description but here is the reference link for this technique
nikkimk
left a comment
nikkimk
left a comment
There was a problem hiding this comment.
Found one example that should probably change in the README.
Also I'm wondering if we should consider moving the Buttons as links docs to typography, and the deprecation notice could be added that redirects the consumers to typography. (But that can wait until the deprecation ticket.)
5t3ph
added
the
Status:Ready for re-review
marissahuysentruyt
left a comment
marissahuysentruyt
left a comment
There was a problem hiding this comment.
I'm super late here, but we talked on Slack about some link-styled-as-button VRT diffs! Apologies for not have a solution or suggestion!
| @@ -23,7 +23,7 @@ export default { | |||
| decorators: [makeOverBackground()], | |||
There was a problem hiding this comment.
Does this also need decorators: [makeOverBackground(staticColor)]?
… into seckles/global-button-link-styling
There was a problem hiding this comment.
- 2nd gen package.json don't explicitly expose global stylesheets. I see its there in 1st-gen. Did we miss this?
- The CSS APIs has no guardrails. If a consumer write the below both the variant selectors will apply and the result will depend on source order in the CSS file. We should document this properly.
<a class="spectrum-Button spectrum-Button--accent spectrum-Button--negative">
Conflicting variants
</a>- Currently
global-elements.cssis auto-loaded intheme.css. Can you do a payload impact assessment if we really have a strong defense against opt-in? - I see a TODO saying "Investigate extending from button" with the new architecture I am a little curious on how the
@layerencapsulation will work. Button's shadow DOM styles and global light-DOM styles have fundamentally different specificity contexts. - Would you be okay to discuss more on the
all: revert-layer !importantpattern, this might have significant implication and we might need to test this with some consumer app that uses Tailwind, CSS modules etc.
| for (const file of glob.sync( | ||
| resolve(__dirname, 'stylesheets/**/*.css') | ||
| )) { | ||
| const dest = resolve(__dirname, 'dist', basename(file)); |
There was a problem hiding this comment.
This means stylesheets/global/global-elements.css and stylesheets/global/global-button.css will both be written to dist/global-elements.css and dist/global-button.css which is flattened but its loosing the global/ directory structure.
There was a problem hiding this comment.
To me that's ok to simplify the export path. I included a directory in our structure for file organization purposes. I wanted to have global directly in the filename so if you were trying to pull up button.css for the component there would be less confusion on which was the correct file (still duplicates due to 1st-gen, so full filepath still matters, of course)
| * governing permissions and limitations under the License. | ||
| */ | ||
|
|
||
| @import url("global-button.css"); |
There was a problem hiding this comment.
This works by accident. But this can break. If you add a top-level stylesheets/global-button.css different from stylesheets/global/global-button.css you will get a collision with no warning.
There was a problem hiding this comment.
Is there a fix you're expecting besides @import url("./global-button.css");?
| "./global-button.css": "./global-button.css", | ||
| "./global-elements.css": "./global-elements.css", |
There was a problem hiding this comment.
Maybe we should a naming contract documentation around this?
There was a problem hiding this comment.
Do you have a suggestion of where that should live?
| */ | ||
|
|
||
| /* TODO: investigage importing styles from Button once migrated */ | ||
| @layer global-elements { |
There was a problem hiding this comment.
Will this name be forever? Its a public contract once shipped.
There was a problem hiding this comment.
Ah - I meant to namespace it, thanks! Will adjust pending team discussion on this technique.
| .swc-Button { | ||
| all: revert-layer !important; | ||
| } |
There was a problem hiding this comment.
I don't think this would scale to multiple global components. This is fine for a single layer but what if you add global-link.css, global-tag.css?
Are you planning to add this for each new global component?
What happens when a consumer wants swc-Button inside a .swc_card? The all: revert-layer !important on .swc-Button could interfere with styles inherited from the card context.
There was a problem hiding this comment.
Custom property overrides are still available, just like in our regular component styling contract. See my Slack thread for us to do further discussion on this topic.
In S2F, the button with icon seems larger than expected to me (20px vs 17px). |
We are exposing the
I don't see this as a burden on us since this is standard CSS behavior and would apply to any class-based library. We could say something like "If you encounter unexpected visual results, ensure only one variant class is applied to the element at a time"?
I re-tested this, and it won't be need added there after all since all content affected is slotted and therefore subject to the light DOM / global stylesheet. To fully be able to use this feature, consumers will have to load the extra stylesheet, which is "opt-in" until the link features are deprecated from button, which is the next phase of this work. 4 & 5 are part of team discussion. |
I understand swc.css is the primary export, but the guide tells consumers to import '@adobe/spectrum-wc/global-elements.css'. This path only works by coincidence of the wildcard export and build flattening. Are you planning to add explicit exports, or is the wildcard pattern intentional? |
| --- | ||
|
|
||
| - **Added**: Introduced global button element styles in `@spectrum-web-components/styles` via `global-button.css` and `global-elements.css` (including public exports), enabling native links with `.spectrum-Button*` classes to render with Spectrum button styling. | ||
| - **Updated**: `@spectrum-web-components/theme` now imports `@spectrum-web-components/styles/global-elements.css` so button-styled native links are styled automatically when used inside `<sp-theme>`. |
There was a problem hiding this comment.
commit 19b5219 reverted this. You might need to change this.
| --- | ||
| '@spectrum-web-components/button': minor | ||
| '@spectrum-web-components/styles': minor | ||
| '@spectrum-web-components/theme': patch |
There was a problem hiding this comment.
No difference in theme.css. Don't think we need to bump this
| await new Promise<void>((res) => { | ||
| setTimeout(() => res()); | ||
| }); | ||
| await (document.fonts ? document.fonts.ready : Promise.resolve()); | ||
| await new Promise<void>((res) => { | ||
| requestAnimationFrame(() => requestAnimationFrame(() => res())); | ||
| }); | ||
| this.ready = true; |
There was a problem hiding this comment.
This is good fix but this change will affect all VRT tests, not just button tests. Was the requestAnimationFrame double-hop specifically needed for stylesheet injection, or is this a general VRT stability improvement you did here? If this is the latter, I would suggest it be better as a separate PR so it can be bisected independently?
6 participants
Description
This initiates a starting place for "global element styles" for both 1st-gen and 2nd-gen, and creates stylesheets for allowing button styles on native, semantic elements, particularly links.
For the 1st-gen component, this forces the swap to a native link when the
hrefattribute has a value. This also prohibitsdisabledorpendingvariants. A separate stylesheet that includes::slotted()selectors enables the styles to be picked up for elements that are direct descendents of<sp-theme>or slotted children of other components.Additionally for 1st-gen, this corrects some display issues in stories related to static color and isolates a style intended only for
min-widthvariants.For 2nd-gen, since Button is not yet migrated, this lays the foundation for enabling global styling. It uses a technique for encapsulating styles via cascade layers to prevent application styles from modifying basic properites like
colorunintentionally. Once migrated, we can investigate extending from button rather than managing an entirely separate stylesheet.Motivation and context
This extends from Nikki's RFC on "Buttons as Links Refactor"
Related issue(s)
Screenshots
Use in 1st-gen as slotted content in Coachmark
Secondary, Icon Only
Use in 2nd-gen as POC in new Guide docs
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Review 1st-gen buttons with href [@cdransf]
hrefvalue swaps to link elementhrefis addedhrefhrefValidate use of 1st-gen buttons with href [@cdransf]
spectrum-Button--secondaryspectrum-Button--iconOnlywith the icon but no labelReview 1st-gen documentation update [@cdransf]
Review 2nd-gen button-styled elements [@cdransf]
Device review
Accessibility testing checklist
Required: Complete each applicable item and document your testing steps (replace the placeholders with your component-specific instructions).
Keyboard (required — document steps below) — What to test for: Focus order is logical; Tab reaches the component and all interactive descendants; Enter/Space activate where appropriate
Screen reader (required — document steps below) — What to test for: Role and name are announced correctly.