[SR] PDF-Space#5976
Conversation
rgclayton
left a comment
rgclayton
left a comment
There was a problem hiding this comment.
😍
One small nit. The top card corner is cut right when it comes into view.
The code is a bit crazy - I think that's sort of expected knowing this is a one off.
overmyheadandbody
left a comment
overmyheadandbody
left a comment
There was a problem hiding this comment.
I admit I couldn't follow the JS logic. It should be clearly documented in the README.md file, so we can make edits in the future, I doubt someone will be able to confidently make tweaks. The result looks great, I do have some CSS notes for now.
| font-size: var(--s2a-font-size-20); | ||
| font-weight: var(--s2a-font-weight-heading); | ||
| line-height: 1.2; | ||
| letter-spacing: -0.4px; |
There was a problem hiding this comment.
It'd be better to use the appropriate heading class, from .heading-super, .heading-1 through .heading-6. You could apply this via JS. Right now the font-size is using a primitive, which is an anti-pattern; the font-weight variable does not exist, it should be --heading-font-family; the line-height and letter-spacing are not using tokens, which will be an issue for maintenance. Applying the appropriate class gets you everything you need from a typography POV.
There was a problem hiding this comment.
good callout! I'll be adding these
| font-size: var(--s2a-font-size-16); | ||
| font-weight: var(--s2a-font-weight-body); | ||
| line-height: 1.5; |
There was a problem hiding this comment.
One of the .body-{size} classes is likely a better fit for consistency and maintenance, similar comment as with headings
| font-size: var(--s2a-font-size-14); | ||
| font-weight: var(--s2a-font-weight-label); |
There was a problem hiding this comment.
A .label class could be leveraged to get you everything you need. Typography elements come with font-size, line-height, letter-spacing and everything the element needs to match the system intent
| .pdf-space .card-scene { | ||
| position: absolute; | ||
| inset: 0; | ||
| z-index: 25; |
There was a problem hiding this comment.
Could the z-index values be flattened a bit? We don't want to risk interference with other elements on the page, like the Gnav, Brand Concierge, etc.
There was a problem hiding this comment.
Technically that should be fine as card-scene would be on a separate stacking context than the rest of the page, so as long as other elements have above 0, they should be able to cover it. It's kind of large and arbitrary now as we also are setting z-index for each card (4 on mobile 8 on tablet/desktop) after figuring out their stacking order. But there should be a way to simplify it further and I'll take a look!
There was a problem hiding this comment.
Updated the values to be more obvious/understandable and also generated comments in css files. Let me know if you have any questions!
| font-size: var(--s2a-font-size-14); | ||
| font-weight: var(--s2a-font-weight-label); |
There was a problem hiding this comment.
Same typography comment, this can likely be a label or have all of the label's declarations copied over.
| border-radius: inherit; | ||
| } | ||
|
|
||
| @media (width >= 768px) and (width <= 1279px) { |
There was a problem hiding this comment.
| @media (width >= 768px) and (width <= 1279px) { | |
| @media (768px <= width < 1280px) |
There was a problem hiding this comment.
great find! Updated
| font-size: var(--s2a-font-size-32); | ||
| /* stylelint-disable-next-line custom-property-pattern */ | ||
| letter-spacing: var(--s2a-font-letter-spacing-neg-0_96); |
There was a problem hiding this comment.
Same typography comment, this could likely leverage a .heading-{X} class
| } | ||
|
|
||
| .pdf-space .acrobat-cta { | ||
| z-index: 18; |
There was a problem hiding this comment.
Another z-index to flatten in the context of this block
|
Note: some of the typography values are still waiting for design to finalize. Currently the block's responsive text behavior might differ from Figma |
5 participants
Add PDF-Space block
Resolves: https://jira.corp.adobe.com/browse/MWPW-192875
Test URLs: