-
Notifications
You must be signed in to change notification settings - Fork 149
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
Chore/vuln deps image focal point #9035
Chore/vuln deps image focal point #9035
Conversation
✅ Deploy Preview for ecommerce-app-base-components canceled.
|
b475682
to
64f4a34
Compare
"homepage": "." | ||
"homepage": ".", | ||
"optionalDependencies": { | ||
"@rollup/rollup-linux-x64-gnu": "4.9.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.
Curious about this too since I've been having some weird rollup issues with optimizely and wondering if you were running into rollup stuff that this fixed?
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.
yeah I was, this fixed it :-)
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 error I was getting that this resolved was
Cannot find module @rollup/rollup-linux-x64-gnu
@@ -143,7 +143,7 @@ export class AppView extends Component { | |||
<> | |||
<div className={styles.background} /> | |||
<div className={styles.body}> | |||
<Typography> | |||
<React.Fragment> |
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 was thinking that React.Fragment
is an old method that was replaced with just the empty brackets <></>
so we probably don't need this? I think Typography
was basically deprecated in Forma for the same reason, that we just use empty fragment tags 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.
Can change 👍
@@ -1,9 +1,8 @@ | |||
import React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import ReactDOM from 'react-dom'; | |||
import { Note } from '@contentful/forma-36-react-components'; | |||
import { Note } from '@contentful/f36-components'; |
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 think one of the updates with note
was the switch to use a generic variant
prop instead of the component specific noteType
prop. I didn't see that change in here and I'm figuring that's because this app isn't in typescript and so it didn't yell about it? But I wonder if the note's variant is actually being respected since we're passing in a deprecated prop.
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! will fix
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! 🥇
Purpose
EXT-5794
Approach
Testing steps
Breaking Changes
Dependencies and/or References
Deployment