Skip to content
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

Pr/new prominence tokens #206

Merged
merged 7 commits into from
Oct 25, 2024
Merged

Pr/new prominence tokens #206

merged 7 commits into from
Oct 25, 2024

Conversation

c4rlosviteri
Copy link
Contributor

Important: PR based on the v4 update

benjag and others added 3 commits September 13, 2023 15:52
Merge next to master
Release next to master
Adds new low-CLS fallback fonts introduced here: rei/rei-cedar#178
@c4rlosviteri c4rlosviteri force-pushed the pr/new-prominence-tokens branch 2 times, most recently from 38fd879 to b04c34c Compare September 6, 2024 18:23
@c4rlosviteri c4rlosviteri force-pushed the pr/new-prominence-tokens branch from b04c34c to 8953eed Compare September 24, 2024 14:06
"CdrProminenceLiftedShadow5Blur": "36px",
"CdrProminenceLiftedShadow5Spread": "0",
"CdrProminenceFlat": [{"color":"#000000","offsetX":"0","offsetY":"0","blur":"0","spread":"0"}],
"CdrProminenceRaised": [{"color":"rgba(75, 74, 72, 0.07)","offsetX":"0","offsetY":"1px","blur":"1px","spread":"0"},{"color":"rgba(75, 74, 72, 0.02)","offsetX":"0","offsetY":"2px","blur":"1px","spread":"0"},{"color":"rgba(75, 74, 72, 0.00)","offsetX":"0","offsetY":"2px","blur":"1px","spread":"0"}],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect this to be a single line, rather than an array—what do you folks think? These variables are definitely less used, but I was under the impression these should be a single line and could be used as-is by a consumer without needing to join an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like a CSS value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya I think so

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my understanding was that these would be used to create a prominence composite is that not the case?

import concat from 'concat';
import path from 'path';
import { getDirname } from '../utils.mjs';
import fs from 'fs-extra'
Copy link
Contributor

@sikhote sikhote Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this file changed? I think the same question applies to a few of the files in this PR. I would like to get Prettier in this library, which would alleviate these types of questions, but we have not added it in yet ☹️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I run eslint to fix those problems. But yeah, I agree. We need Prettier. Do you want me to add it in this PR too?

Copy link
Contributor

@sikhote sikhote Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets get it in the next one after this gets merged in, it's ok as-is

type: 'value',
transitive: true,
filter: (token) => token.$type === 'shadow',
transform: (token) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

Copy link
Contributor

@sikhote sikhote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 💯 thanks for making that update.

@c4rlosviteri c4rlosviteri merged commit a6c0997 into next Oct 25, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants