-
Notifications
You must be signed in to change notification settings - Fork 0
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
coreui NoteBox component #1276
base: main
Are you sure you want to change the base?
coreui NoteBox component #1276
Changes from 4 commits
8e04c82
8c7ffcc
f4e7ff3
de3722b
99065db
7b84bdc
8f1e79c
888030e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
import { css } from '@emotion/react'; | ||
import React, { ReactNode } from 'react'; | ||
import { error, warning, mutedBlue } from '../../definitions/colors'; | ||
|
||
export interface Props { | ||
type: 'info' | 'warning' | 'error'; | ||
children: ReactNode; | ||
} | ||
|
||
const baseCss = css` | ||
border-left: 0.25em solid var(--note-box-border-color); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ive never seen this before. That's very slick |
||
border-radius: 0.25em; | ||
padding: 0.5em 1em; | ||
background: var(--note-box-bg-color); | ||
gap: 1em; | ||
margin-bottom: 1em; | ||
`; | ||
|
||
const infoCss = css` | ||
--note-box-bg-color: ${mutedBlue[100]}; | ||
--note-box-border-color: ${mutedBlue[600]}; | ||
${baseCss}; | ||
`; | ||
|
||
const warningCss = css` | ||
--note-box-bg-color: ${warning[100]}; | ||
--note-box-border-color: ${warning[600]}; | ||
font-weight: 500; | ||
${baseCss}; | ||
`; | ||
|
||
const errorCss = css` | ||
--note-box-bg-color: ${error[100]}; | ||
--note-box-border-color: ${error[600]}; | ||
font-weight: 500; | ||
${baseCss}; | ||
`; | ||
|
||
export function NoteBox(props: Props) { | ||
const finalCss = | ||
props.type === 'warning' | ||
? warningCss | ||
: props.type === 'error' | ||
? errorCss | ||
: infoCss; | ||
return <div css={finalCss}>{props.children}</div>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps for later, but it'd be nice to pair the icon with the NoteBox type. We picked one icon for error, warning, and info ( a looong time ago and i think it was never actually impllemented). Of course, one could pass that as part of the child, but if we want to keep coreui very opinionated then there could be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed! |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
import React from 'react'; | ||
import { NoteBox, Props } from '../../components/containers/NoteBox'; | ||
import { Story, Meta } from '@storybook/react/types-6-0'; | ||
import { UIThemeProvider } from '../../components/theming'; | ||
import { mutedGreen, mutedMagenta } from '../../definitions/colors'; | ||
|
||
export default { | ||
title: 'Containers/NoteBox', | ||
component: NoteBox, | ||
} as Meta; | ||
|
||
const Template: Story<Props> = function Template(props) { | ||
return ( | ||
<UIThemeProvider | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason to add the theme here? Or is it just to check that we get the correct colors regardless of theme? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just following the same pattern as another story file (can't remember which one). I guess we could take it out, but not sure it hurts? |
||
theme={{ | ||
palette: { | ||
primary: { hue: mutedGreen, level: 500 }, | ||
secondary: { hue: mutedMagenta, level: 500 }, | ||
}, | ||
}} | ||
> | ||
<NoteBox {...props} /> | ||
</UIThemeProvider> | ||
); | ||
}; | ||
|
||
export const Info = Object.assign(Template.bind({}), { | ||
args: { | ||
type: 'info', | ||
children: ( | ||
<div> | ||
This is some general information about the content that follows on the | ||
page. | ||
</div> | ||
), | ||
}, | ||
}); | ||
|
||
export const Warning = Object.assign(Template.bind({}), { | ||
args: { | ||
type: 'warning', | ||
children: ( | ||
<div>This is a warning about the content that follows on the page.</div> | ||
), | ||
}, | ||
}); | ||
|
||
export const Error = Object.assign(Template.bind({}), { | ||
args: { | ||
type: 'error', | ||
children: ( | ||
<div> | ||
This is an error message about the content that follows on the page. | ||
</div> | ||
), | ||
}, | ||
}); | ||
|
||
export const LongContent = Object.assign(Template.bind({}), { | ||
args: { | ||
type: 'info', | ||
children: ( | ||
<div> | ||
Lorem ipsum odor amet, consectetuer adipiscing elit. Faucibus morbi ac | ||
ultrices purus urna tristique mattis consequat. Posuere volutpat | ||
facilisi natoque dictumst dignissim magna dapibus. Taciti vel a etiam | ||
curabitur velit torquent. Fusce interdum dictum vulputate sollicitudin | ||
nulla. Orci placerat congue odio aptent enim mauris. Turpis nec rhoncus | ||
eleifend eleifend eget. Auctor sed nullam vestibulum quisque egestas; | ||
nullam aenean ante. | ||
</div> | ||
), | ||
}, | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A story using a different type of child (for example, an image or some other non-text element) would be helpful if time allows There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or an expandable one There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I agree! I will add a couple more, soon. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ import { PfamDomain } from 'ortho-client/components/pfam-domains/PfamDomain'; | |
import { | ||
FilledButton, | ||
FloatingButton, | ||
NoteBox, | ||
OutlinedButton, | ||
SelectList, | ||
Undo, | ||
|
@@ -662,23 +663,7 @@ export function RecordTable_Sequences( | |
} as CSSProperties | ||
} | ||
> | ||
{warningText && ( | ||
<div | ||
style={{ | ||
display: 'flex', | ||
flexWrap: 'wrap', | ||
borderLeft: '.2em solid rgb(225, 133, 133)', | ||
borderRight: '.2em solid rgb(225, 133, 133)', | ||
padding: '.5em 1em', | ||
background: 'rgb(255, 228, 228)', | ||
gap: '1em', | ||
marginBottom: '1em', | ||
fontWeight: 500, | ||
}} | ||
> | ||
{warningText} | ||
</div> | ||
)} | ||
{warningText && <NoteBox type="error">{warningText}</NoteBox>} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should either change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is one of those things where someone wanted the box to be red, even though it isn't strictly an error. I will change it to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
<div | ||
style={{ | ||
padding: '10px', | ||
|
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.
Perhaps additionally a default font? I believe coreui generally uses Inter or Roboto
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 content, I think we should use the same font as the rest of the site, but I will take a look at using one of these other fonts.
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 I'm going to leave the font as-is.