-
Notifications
You must be signed in to change notification settings - Fork 151
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: update to the latest forma and new tokens #336
Conversation
suevalov
commented
Jul 26, 2021
•
edited
Loading
edited
- Bumps Forma36 to the latest version
- Update all tokens values (using codemod)
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 naming vs current values are a bit confusing e.g: ElementMid
< TextMid
but overall code looks fine. I guess colorPrimary
doesn't need changing?
I would manual check also on how apps look with these new values @fabe
50c5202
to
7877a6e
Compare
Checked these apps (don't have access to all of them):
|
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.
Left a bunch of minor questions but LGTM so far as long as tests pass
@@ -13,6 +13,9 @@ describe("EditorPage", () => { | |||
beforeEach(() => { | |||
sdk = mockProps.sdk; | |||
sdk.entry.fields.experimentId.onValueChanged = fn => () => {}; | |||
|
|||
Date.now = jest.fn(() => 100); |
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 reason for this change? 🤔
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.
Tests failed due to tricky date and random number mock logic inside the Dropdown component:
https://github.com/contentful/forma-36/blob/99609176a0604fd0f6b62753cf45a246cdbb48b6/packages/forma-36-react-components/src/components/Dropdown/Dropdown.test.tsx#L11-L14
Ids are generated based on random number, so needed to stub it here too. Checked this in with @suevalov
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.
Good to know, thanks!
@@ -185,7 +185,7 @@ export default class AppConfig extends React.Component<Props, State> { | |||
); | |||
} | |||
|
|||
onParameterChange = (key: string, e: React.ChangeEvent<HTMLInputElement>) => { | |||
onParameterChange = (key: string, e: React.ChangeEvent<HTMLInputElement | HTMLTextAreaElement> | React.ChangeEvent<HTMLSelectElement>) => { |
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 thing here - do we support more elements? I dont see an equivalent change in HTML? I still see the Select tag
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.
Type error comes from TextField
using onParameterChange
handler:
onChange={this.onParameterChange.bind(this, def.id)} |
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.
Good call. Maybe then removing the HTMLInputElement
?
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.
But this types comes from forma-36 type definition:
https://github.com/contentful/forma-36/blob/master/packages/forma-36-react-components/src/components/TextField/TextField.tsx#L35
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.
Please recreate the package-lock.json
files with npm 6. The files were (accidentally?) migrated to lockfileVersion
2.
I am also happy to upgrade the whole repo to npm 7 but I don't think this PR is the right place for that.
Lock files have the correct version now
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.
LGTM - but haven't done any UI check