-
Notifications
You must be signed in to change notification settings - Fork 0
CORE-1085: Adding Banner folder #102
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
Conversation
@@ -1,4 +1,4 @@ | |||
import { MessageBox } from './MessageBox'; |
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 a reorganization of MessageBox into a folder
sorry for the delay on this. a couple notes after just skimming: I don't think we want separate style files. the components directory is already pretty bloated so unless the styles add a ton of lines of code I would keep them with the component.
|
DismissIcon can go in the |
src/index.ts
Outdated
export * from './components/MessageBox.styles'; | ||
export * from './components/Html'; | ||
export * from './components/MessageBox/MessageBox'; | ||
export * from './components/svgs/DismissIcon'; |
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 don't think you need to export it - it can live here and in individual projects
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.
GracePeriodNotification
uses DismissIcon, that is why I put it as an export. But yeah, I can create it as well in assignments
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 a little redundancy is ok (and I wouldn't even bother creating a new component in assignments unless we start using the icon all over the place)
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.
import { Banner } from './Banner'; | ||
import styled from 'styled-components'; | ||
|
||
const BannerContainer = styled.div` |
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 to leave this out of the component itself?
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.
Because I don't see that's part of assignments. If a put it inside the component it could break how old looks in assignments. I tried to leave everything the same
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.
Let me know if you think it's okay. @bethshook
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 think that's fine, as long as it looks as expected when you import it to assignments
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.
Sure, I made some tests in assignments and everything looks like before the import from ui-components!
CORE-1085
Banner is a migration of Warning from assignments to ui-components.
Review the Storybook to know if everything is ok with styles.