-
Notifications
You must be signed in to change notification settings - Fork 198
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
chore(search): add missing size-specific CSS properties #3538
base: main
Are you sure you want to change the base?
Conversation
|
🚀 Deployed on https://pr-3538--spectrum-css.netlify.app |
File metricsSummaryTotal size: 2.25 MB*
search
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
819727b
to
f589fec
Compare
I am getting a Typography regression in VRT which seems flaky (?), not sure if this is known / how to proceed here! https://www.chromatic.com/build?appId=64762974a45b8bc5ca1705a2&number=4191 |
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.
Looks good - just leaving a quick question here for you!
components/search/themes/express.css
Outdated
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 addition of --spectrum-search-block-size
, --spectrum-search-icon-size
, and --spectrum-search-text-to-icon
seem a little repetitive here in Express, on the CSS side of things as far as I can tell, those custom properties look like they're being inherited in Express from where they're set in spectrum-two.css
, would things still work as you'd want them in SWC if we removed the extra code you added here in express.css
?
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.
express.css
inherits spectrum.css
if I am not wrong! Can you please check @rise-erpelding
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.
Correct, spectrum-two.css
imports spectrum.css
, and express.css
imports spectrum.css
.
You can set, for instance, --spectrum-foo: var(--spectrum-bar)
in spectrum-two.css
and still see that being applied in Express if it's not been overridden in spectrum
or express
. Unless we need to leave it in to avoid an issue in SWC, I think we could probably remove the custom properties that are redefined in express.css
since those tokens are the same for all themes.
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.
Yes this make sense. Thanks
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'm wondering why this is needed in SWC, when those same values for S/L/XL are already being set in the index.css
, which should apply to all themes?
http://github.com/adobe/spectrum-css/blob/main/components/search/index.css#L93-L109
If an increase in specificity is needed, would that be done in the linked section above instead?
Do you have a testing link where the screenshotted issue is present? When I go to the Storybook at the linked PR 5085, I do not see that issue appearing.
Description
Added missing size-specific CSS custom properties to the Search component in both Spectrum 2 and Express themes. These properties are required by downstream consumers (like Spectrum Web Components) to properly render the component across different sizes.
How and where has this been tested?
Even though this change is not reflected in this project (no VRT changes), it will cascade and fix undefined variables in Spectrum Web Components
You can see the properties not defined on the initial runs of adobe/spectrum-web-components#5085, resulting in a broken component design. This change makes it so the CSS properties are defined in the SWC's
#textfield
element which will then render it correctly (tested manually by overriding the local CSS in SWC).Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.
Validation steps
node_modules
yarn start
and verify the http://localhost:8080/storybook/index.html?path=/story/search-sizes--xl renders correctly.Regression testing
Validate:
Screenshots
Broken component in SWC:

To-do list