-
Notifications
You must be signed in to change notification settings - Fork 199
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
feat(backgroundlayers): class based implementation #2274
Conversation
945e402
to
cf92f21
Compare
🚀 Deployed on https://pr-2274--spectrum-css.netlify.app |
Unless required by applicable law or agreed to in writing, software distributed under | ||
the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS | ||
OF ANY KIND, either express or implied. See the License for the specific language | ||
governing permissions and limitations under the License. |
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 intentionally using a bit of a different structure to the css here so that you can access these by just using one class spectrum-BackgroundLayers--elevated
instead of spectrum-BackgroundLayers spectrum-BackgroundLayers--elevated
I am also doing a bit of hard coding, the border color, drop shadow, and dark colors, these would need to replaced with tokens.
name: Browsing Contexts | ||
markup: | | ||
<div class="spectrum-Examples" style="justify-content: flex-start; position: relative; height: 150px;"> | ||
<div class="spectrum-BackgroundLayers--elevated" style="inline-size: 100px; block-size: 100px; border-radius: 10px; position: absolute; z-index: 4;"> |
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.
using inline styling here to keep these out of the classes
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.
Yep that seems like the right move 👍
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 really like the direction you're headed with this. I left some typo nitpicks and I think we should consider how we want to document this utility class idea moving forward. I like the choice to use only one class (vs like spectrum-BackgroundLayers spectrum-BackgroundLayers--elevated
).
To your question:
Should the border radius and border come with this or is that going to vary based on implementation?
From a dev perspective we can probably include them and consumers can adjust with --mod
s if they need to. I'm interested in thoughts on this from a design perspective as well...but I think the designs for this may be finalized? Which makes me think that as of right now that's the only spec that'll be carried forward
@@ -0,0 +1,7 @@ | |||
# @spectrum-css/backgroundlayers |
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're sort of undecided about documentation moving forward (re: migration guidelines convo) but this might be a good place for now to document this class system once we get to a more final place with it... I also think we need documentation in Storybook and the docs site so maybe we can do some brainstorming about what that looks like. Docs site might be more straightforward than Storybook
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 did add some documentation to the readme. I'd agree that there is opportunity to keep brain storming what we want documentation to look like.
name: Browsing Contexts | ||
markup: | | ||
<div class="spectrum-Examples" style="justify-content: flex-start; position: relative; height: 150px;"> | ||
<div class="spectrum-BackgroundLayers--elevated" style="inline-size: 100px; block-size: 100px; border-radius: 10px; position: absolute; z-index: 4;"> |
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.
Yep that seems like the right move 👍
components/tokens/package.json
Outdated
@@ -27,7 +27,7 @@ | |||
"format:results": "prettier --no-config --no-error-on-unmatched-pattern --ignore-unknown --loglevel silent --write dist/" | |||
}, | |||
"devDependencies": { | |||
"@adobe/spectrum-tokens": "12.22.1", | |||
"@adobe/spectrum-tokens": "0.0.0-s2-20230810215532", |
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.
Ohh is this so we can use the S2 tokens?
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, at least for now, I took this from the PR Patrick put up to test the s2 changes
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.
Yep! The one in the diff above is from one of the first snapshots that was released. Garth has also been releasing newer versions against the 13.0.0
number here: https://github.com/adobe/spectrum-tokens/releases - anything that's a beta
for 13.0.0
is S2-related.
components/backgroundlayers/stories/backgroundlayers.stories.js
Outdated
Show resolved
Hide resolved
ac72ee6
to
67fe7c2
Compare
File metricsSummaryTotal size: 4.07 MB*
Detailsbackgroundlayers
modal
tokens
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
} | ||
|
||
background-color: var(--spectum-backgroundlayers-background-color); | ||
filter: drop-shadow( |
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.
Is there a benefit to using filter: drop-shadow
instead of box-shadow
? I am thinking about if the class is ever applied to an image, the shadow will follow the contours of the alpha channel, which may or may not be desirable.
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.
thanks for suggesting this, I've changed it to a box-shadow
BREAKING CHANGE: Merges Express and Spectrum to create Spectrum 2
Includes new color values for Spectrum 2
53074ad
to
19773dc
Compare
* fix(stepper): no longer touches InfieldButton classes * fix(stepper): button size + space match infieldbutton spec
d8e6964
to
5d49521
Compare
5ec70af
to
c4aa5f5
Compare
Description
Experimental PR to look at adding Background Layers
This approach creates utility classes which can be used by adding background layers to the dependencies for the component and then using the classes where needed.
Changes:
nav.pug
file.spectrum-BackgroundLayers--elevated
,spectrum-BackgroundLayers--layer2
,spectrum-BackgroundLayers--layer1
,spectrum-BackgroundLayers--pasteboard
, andspectrum-BackgroundLayers--base
Look at the docs site for Alert Dialog to see the elevated class in use
Storybook