-
Notifications
You must be signed in to change notification settings - Fork 200
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(dropzone): migrating new S2 tokens, more spaced out dashed lines, more illustrations #3429
feat(dropzone): migrating new S2 tokens, more spaced out dashed lines, more illustrations #3429
Conversation
🦋 Changeset detectedLatest commit: 7f5e9c8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
b244774
to
230c342
Compare
70c8b73
to
b2b714d
Compare
5a2a335
to
27ac354
Compare
e7f55d8
to
654db3d
Compare
ac120bd
to
8894290
Compare
491b648
to
c608159
Compare
🚀 Deployed on https://pr-3429--spectrum-css.netlify.app |
File metricsSummaryTotal size: 1.37 MB*
dropzone
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
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.
Gosh, this one was a lot of work too! Leaving some initial thoughts and some questions about the CSS and design spec.
I think you might have made a couple of pushes after I started reviewing, so pardon if some of the comments made aren't up to date!
components/dropzone/index.css
Outdated
inline-size: 101%; | ||
block-size: 101.5%; |
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.
How are these numbers calculated?
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 was to make the stroke of the svg/rect fit. I think I can bring this back to 100% now that I figured out how the radius of the svg rectangle works
56c6806
to
793571c
Compare
5841e1d
to
f070d25
Compare
db2a5dd
to
f34473b
Compare
cf1337c
to
940d10c
Compare
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 work on this! This one was pretty hairy and super intwined with illustrated message. Thanks for cleaning it all up!
I have some questions that I didn't quite know where to put...
- My first question is about using the
mod-drop-zone-width
. If I set that, the inner content is then uncentered. Should we add
display: flex;
align-items: center;
justify-content: center;
to .spectrum-DropZone
to prevent/combat that from happening? I guess I'm thinking that if a user needs to use that mod, I wouldn't want them to also have to go into the CSS to recenter the content. I'd just want them to be able to set the mod, and it just works for them. Does that make sense? Is that even a valid concern?

- I also wanted to double check the color of the upload SVG visual. You've got it set correctly I think, to neutral-content-color-default (an alias for gray-800) like the Figma specs say. But Figma is actually using neutral-visual-color (which is an alias for gray-500) instead. You may have already checked on this, but which one is design expecting? There's definitely a difference since those 2 grays are quite distinct from each other.
- Last thing I'm curious about is the button color for
isDragged+isFilled
. Currently, the background of the button in theisDragged+isFilled
state is set toaccent-visual-color
like the design specs. 👍 However...that token is resolving to a different rgb in the browser when I compare between Chrome and Figma. Is that design token incorrect? It seems like that token (maybe in tandem with the background color opacity?) is causing the button colors to change- is that correct? If I remove thebackground
style from.spectrum-DropZone-button
they resolve to the same rgb, but _notaccent-visual-color
. (it's--spectrum-accent-background-color-default
🤦♀️)
Lemme know if you wanna pair through any of this! I know I left a lot of ideas and questions for you!
components/pagination/CHANGELOG.md
Outdated
@@ -325,7 +325,7 @@ Output for all component CSS files is now being run through a lightweight optimi | |||
|
|||
### 🛑 BREAKING CHANGES | |||
|
|||
- Replaces DNA tokens with Spectrum tokens. | |||
- Replaces DNA tokens with Spectrum 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.
I'm also not sure we need to change anything for pagination in this PR. If it were me, I might drop this, but I'll leave it up to you since it's not hurting anything (and arguably makes it better).
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.
@aramos-adobe I still see these changes in the latest pull of this branch. I don't think it's really necessary to make this change in the context of this PR, that's all. It's not related to the rest of the work, but I'll leave it up to you if you want to keep it 👍
components/dropzone/index.css
Outdated
--mod-drop-zone-content-height: 280px; | ||
|
||
/* Typography */ | ||
--mod-illustrated-message-title-font-weight: var(--spectrum-title-serif-font-weight); | ||
} | ||
|
||
.spectrum-DropZone { | ||
box-sizing: border-box; | ||
inline-size: var(--mod-drop-zone-width, var(--spectrum-drop-zone-width)); |
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 isn't anything you introduced, but I believe the preference is to not use the token (in this case, drop-zone-width
) in a direct style definition like this. I think we would need to define a custom property up towards the top of the file with the padding and border width, etc. and then use that in this style definition.
.spectrum-Dropzone {
--spectrum-drop-zone-padding: var(--spectrum-spacing-400);
--spectrum-drop-zone-border-width: var(--spectrum-border-width-200);
--spectrum-drop-zone-corner-radius: var(--spectrum-corner-radius-700);
--spectrum-drop-zone-border-color: var(--spectrum-gray-300);
--spectrum-drop-zone-inline-size: var(--spectrum-drop-zone-width); // use the token here just to define the custom prop
... // all of the other custom props & mods....
}
.spectrum-Dropzone {
box-sizing: border-box;
inline-size: var(--mod-drop-zone-inline-size, var(--spectrum-drop-zone-inline-size)); // then use the custom prop in the style definition
... // and all the rest of the styles
This could be a nice way to "leave it better than we found it." Thoughts?
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 addressed the color of the illustration last week in a thread. Design team is updating the Figma to match the token colors
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 looks really good! I left some really small comments, but I don't think any of them are blocking, so I'll approve 👍 Sounds like there's some updates that are needed on the Figma side, but the code is all pointing to the right values & tokens.
tokens/custom/global-vars.css
Outdated
--spectrum-examples-gradient-static-black: linear-gradient(45deg, rgb(255 241 246), rgb(238 245 255)); | ||
--spectrum-examples-gradient-static-white: linear-gradient(45deg, rgb(64 0 22), rgb(14 24 67)); |
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.
@aramos-adobe I still see these changes. Do we need them?
components/pagination/CHANGELOG.md
Outdated
@@ -325,7 +325,7 @@ Output for all component CSS files is now being run through a lightweight optimi | |||
|
|||
### 🛑 BREAKING CHANGES | |||
|
|||
- Replaces DNA tokens with Spectrum tokens. | |||
- Replaces DNA tokens with Spectrum 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.
@aramos-adobe I still see these changes in the latest pull of this branch. I don't think it's really necessary to make this change in the context of this PR, that's all. It's not related to the rest of the work, but I'll leave it up to you if you want to keep it 👍
}; | ||
|
||
/** | ||
* The filled state of a drop zone is used to indicate that a file has been dropped into the drop zone. Since the type size doesn't change in this state the dropzone size would be identical in all size variants. |
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.
A comma may be appropriate in the last sentence here. This is a very small, non-blocking nitpick.
* The filled state of a drop zone is used to indicate that a file has been dropped into the drop zone. Since the type size doesn't change in this state the dropzone size would be identical in all size variants. | |
* The filled state of a drop zone is used to indicate that a file has been dropped into the drop zone. Since the type size doesn't change in this state, the dropzone size would be identical in all size variants. |
class=${classMap({ | ||
size = "m", | ||
image = "dropzone-illustration.png", | ||
...globals |
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.
Oh this isn't something I noticed during the last review. Are we starting to transition away from context
and back to ...globals
? Since the spectrum-two
branch doesn't have the S1 or Express contexts?
I would say revert this back to using the context = {}
stuff (as well as the ...globals
in the IllustratedMessage()
down further), but are we even storing context data anymore? I don't actually know 🙃
Dropzone S2 Migration
Since dropzone shares a lot of illustrated message's new content tokens (typography and spacing), tons of unused
--mods
were removed.The component reflects the new design of the borders that includes dash length and dash gap.
SVG Border
To support the intention of the design, an SVG element is used. There modifiable tokens to adjust the length of dashed lines and the gap between them. It's understood it may be too much to consume an additional element so there's a fallback where the SVG element is not necessary - dropzone will use the standard
border
CSS property.New mods
--mod-drop-zone-content-height
Removed mods
--mod-drop-zone-body-color
--mod-drop-zone-body-font-family
--mod-drop-zone-body-font-style
--mod-drop-zone-body-font-weight
--mod-drop-zone-body-line-height
--mod-drop-zone-content-color
--mod-drop-zone-content-edge-to-text
--mod-drop-zone-content-font-size
--mod-drop-zone-heading-color
--mod-drop-zone-heading-font-family
--mod-drop-zone-heading-font-size
--mod-drop-zone-heading-font-style
--mod-drop-zone-heading-font-weight
--mod-drop-zone-heading-line-height
--mod-drop-zone-heading-to-body
--mod-drop-zone-illustration-to-heading
--mod-drop-zone-illustration-to-title
How and where has this been tested?
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
replace
<svg className="spectrum-DropZone-stroke">
Regression testing
Validate:
Screenshots
To-do list