-
Notifications
You must be signed in to change notification settings - Fork 47
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(tokens|common): add typescript #669
π feat(tokens|common): add typescript #669
Conversation
This reverts commit d675171.
eedab9b
to
0bdaff2
Compare
packages/common/src/elevate.ts
Outdated
if (level) { | ||
return all[level]; | ||
} | ||
|
||
return all; |
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 previous way seemed more clear, i would only using nullish coalescing operator. WDYT?
if (level) { | |
return all[level]; | |
} | |
return all; | |
all[level] ?? all; |
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.
Hey, @MicaelRodrigues!
I don't think the previous way is more clear. Someone new to JS may have difficulty to understand all[level] ?? all
. Contrary to what you said, I believe that this new implementation is easier to read and to understand the code.
Don't you think it's easier to read the statements if we were reading like it was an English text, instead of a code full of JS patterns and operators?
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.
honestly, I understand both points but there's two things that seem more important to me here:
- what if I give it a higher level? Right now, as we accept any number I could give it a 500 and would get an undefined. Should we create the array based on the given level?
- this function signature is, IMO, weird. It can either return an array or a value. I'm not the biggest fan of doing this as it makes the usage a lot more complex. Maybe we should type the return differently based on the fact that a level is given or not using function overloads
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.
Hey, @MicaelRodrigues! I don't think the previous way is more clear. Someone new to JS may have difficulty to understand
all[level] ?? all
. Contrary to what you said, I believe that this new implementation is easier to read and to understand the code.Don't you think it's easier to read the statements if we were reading like it was an English text, instead of a code full of JS patterns and operators?
- could give it a 500 and would get an undefined. Should we create the arr
It's a question of flavour :) Both options are fine to me :) We expect people working in this code to know javascript fundamentals. If they don't we are here to help improve the code... :)
I agree with you in "as it was an English text" and both options read plain english to me (ex: my suggestion reads ("a" or "b").
Anyway, the important concerns in this function are the ones @goncalo-matos exposed ;)
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.
Can we use the value "0" as level
? If so the current implementation will ignore the level when it is "0".
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 @leonardokl π
Sorry about this guys!
@goncalo-matos good point π
@@ -0,0 +1,27 @@ | |||
export interface FontSizesProps extends Array<number> { |
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.
Same as above
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.
Same as above, you don't need extends
packages/common/src/elevate.ts
Outdated
if (level) { | ||
return all[level]; | ||
} | ||
|
||
return all; |
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.
Hey, @MicaelRodrigues!
I don't think the previous way is more clear. Someone new to JS may have difficulty to understand all[level] ?? all
. Contrary to what you said, I believe that this new implementation is easier to read and to understand the code.
Don't you think it's easier to read the statements if we were reading like it was an English text, instead of a code full of JS patterns and operators?
margin: 71, | ||
gutter: 24, | ||
}, | ||
}; |
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.
}; | |
} as const; |
So we can make the IntelliSense and the Editor/autocomplete smarter
|
||
/** | ||
* @type {Color} | ||
*/ | ||
const white = '#FFFFFF'; | ||
|
||
const 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.
export default colors = {
vibin,
hope,
energy,
relax,
peace,
brandingVerve,
verve,
uplift,
deepPurple,
yoga,
success,
neutral,
attention,
stamina,
deep,
medium,
light,
clear,
white,
} as const;
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.
For the future, not for now, we should probably avoid exporting default. we're polluting the bundle with all of the colors if we only use one
(named exports scale a lot better than default exports :P)
"build": "yarn build:cjs && yarn build:esm", | ||
"build:cjs": "NODE_ENV=cjs babel ./src --out-dir dist/cjs", | ||
"build:esm": "NODE_ENV=esm babel ./src --out-dir dist/esm", | ||
"build": "yarn build:types && yarn build:cjs && yarn build:esm", |
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.
suggestion: wdyt of parallelizing this? Something like https://www.npmjs.com/package/npm-run-all can be helpfull
packages/common/src/elevate.ts
Outdated
function elevate({ | ||
interface ElevateProps { | ||
color?: string; | ||
level?: number; |
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.
suggestion: since this only supports up to 3, shouldn't our types reflect this?
quick and dirty way would be to do this (we could build a Range util later):
level?: number; | |
level?: 0 | 1 | 2 | 3 |
packages/common/src/elevate.ts
Outdated
if (level) { | ||
return all[level]; | ||
} | ||
|
||
return all; |
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.
honestly, I understand both points but there's two things that seem more important to me here:
- what if I give it a higher level? Right now, as we accept any number I could give it a 500 and would get an undefined. Should we create the array based on the given level?
- this function signature is, IMO, weird. It can either return an array or a value. I'm not the biggest fan of doing this as it makes the usage a lot more complex. Maybe we should type the return differently based on the fact that a level is given or not using function overloads
|
||
const border: BorderProps = [0, 1, 2]; | ||
|
||
[border.zero, border.small, border.medium] = border; |
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.
suggestion: I think we can remove this line. I'm not really following what it's doing π€
@@ -0,0 +1,101 @@ | |||
export interface BreakpointsProps { |
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.
suggestion: these aren't really props. How about just Breakpoints?
export interface BreakpointsProps { | |
export interface Breakpoints { |
|
||
/** | ||
* @type {Color} | ||
*/ | ||
const white = '#FFFFFF'; | ||
|
||
const 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.
For the future, not for now, we should probably avoid exporting default. we're polluting the bundle with all of the colors if we only use one
(named exports scale a lot better than default exports :P)
}; | ||
|
||
const [zero, small, medium, large] = | ||
elevate(elevateOptions) as [string, string, string, string]; |
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.
question: as
casts hide potential errors (it's us saying we know better than typescript). Do we really need this?
@@ -0,0 +1,27 @@ | |||
{ | |||
"compilerOptions": { | |||
"target": "es2016", |
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.
question: could we make this a bit more recent? :P
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.
Fuel to the fire :P
https://www.freecodecamp.org/news/javascript-new-features-es2020/
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.
For sure!
But I think it's better to do this update at another time, as there would be a lot to validate in this PR
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.
For sure!
But I think it's better to do this update at another time, as there would be a lot to validate in this PR
I don't think there would be much to validate in this PR, did you test the change locally? If indeed causes much trouble we can do in another PR
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.
@matheushdsbr Pls don't forget to open a new PR with this :)
"forceConsistentCasingInFileNames": true, | ||
"allowSyntheticDefaultImports": true, | ||
"strict": true, | ||
"noImplicitAny": false, |
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.
one day, I'd love to have this as true :D
@MicaelRodrigues @goncalo-matos @leonardokl @caiotracera Hey guys! When changing the structure of the tokens by removing the arrays, this test broke: Investigating the I believe that it would be better, at least for now, to continue with the old structure (with arrays) and in the future to investigate solutions to remove this structure and update with suggestions that you commented here. I believe I wasn't very clear, I still don't quite understand how this function works. I would like your opinion on the case! |
I see, we need to keep "array" as the return in order to prevent a breaking change.
|
This reverts commit 9937fbf.
Great suggestion @MicaelRodrigues!! ππ But when testing I found a problem, the old structure [with array] allows us to use tokens in two ways:
All consoles were made in this test file:
Consoling the old structure with arrays:
With your suggestion we can use just like:
Code
Result
But!!! Your suggestion made me think better and I thought of this way:
Result:
With that way we can continue using the same tokens as in the array structure:
\o/ |
@@ -0,0 +1,27 @@ | |||
{ | |||
"compilerOptions": { | |||
"target": "es2016", |
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.
@matheushdsbr Pls don't forget to open a new PR with this :)
Description π§π»ββοΈ
In this PR was added Typescript in
tokens
andcommon
packages.Platforms π²
Type of change π
How Has This Been Tested? π§ͺ
[Enter the tips to test this PR]
Checklist: π
Video πΈ
Screen.Recording.2023-07-12.at.17.20.15.mov