-
Notifications
You must be signed in to change notification settings - Fork 142
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
fix(Pageheader,Tearsheet,Notifications): resolves CSP violations #6340
fix(Pageheader,Tearsheet,Notifications): resolves CSP violations #6340
Conversation
✅ Deploy Preview for ibm-products-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-for-ibm-products ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Interesting, thank you! Matt to the rescue 🛟 |
@@ -31,7 +31,7 @@ import { | |||
Copy, | |||
ErrorFilled, | |||
CheckmarkFilled, | |||
} from '@carbon/react/icons'; | |||
} from '@carbon/icons-react'; |
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.
Not sure about this change? All icons should now be imported from @carbon/react/icons
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.
It was giving me a build error and somehow changing the import fixes 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.
Can you share what the error was?
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.
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.
When importing from @carbon/icons-react
it fixes
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.
We had a convo with Carbon core a while back and I don't know where we landed with this, FYI @tay1orjones
packages/ibm-products/src/components/ConditionBuilder/ConditionBuilder.types.ts
Show resolved
Hide resolved
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.
@matthewgallo fixed page header styles ! |
Sorry, one more style change! I just noticed that the entrance animation for the notification panel is also missing with the current changes. |
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.
Just spotted one more style change in the stacked tearsheets that looks unintended.
I see its the same issue when using CSS properties, nice catch |
@matthewgallo fixed, i assume it was the size of the tearsheet? |
packages/ibm-products/src/components/NotificationsPanel/NotificationsPanel.tsx
Outdated
Show resolved
Hide resolved
4e11b90
Closes #6190
This PR resolves CSP violation issues
What did you change?
Moved inline styles to
useIsomorphicEffect
How did you test and verify your work?
Storybook