-
Notifications
You must be signed in to change notification settings - Fork 6
Alert Component #86
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
Alert Component #86
Changes from all commits
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,48 @@ | ||
| import React, { useRef, useEffect} from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
| const Alert = (props) => { | ||
| const hxRef = useRef(null); | ||
|
|
||
| useEffect(() => { | ||
| hxRef.current.addEventListener('open', props.onOpen); | ||
| hxRef.current.addEventListener('close', props.onClose); | ||
| return () => { | ||
| hxRef.current.removeEventListener('open', props.onOpen); | ||
| hxRef.current.removeEventListener('close', props.onClose); | ||
| }; | ||
| }, []); | ||
|
|
||
| return ( | ||
| <> | ||
michaelmang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| {/* | ||
| Wrappping element needed: Otherwise when alert removes itself from DOM on close it confusing React | ||
| about where highest level parent element went, and will throw an error. | ||
| */} | ||
| <hx-alert | ||
| type={props.type} | ||
| status={props.status} | ||
| persist={props.persist} | ||
| cta={props.cta} | ||
| className={props.className} | ||
| id={props.id} | ||
| ref={hxRef} | ||
| > | ||
| {props.children} | ||
| </hx-alert> | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| Alert.propTypes = { | ||
| className: PropTypes.string, | ||
| children: PropTypes.node.isRequired, | ||
| type: PropTypes.string, | ||
| status: PropTypes.string, | ||
| cta: PropTypes.string, | ||
| persist: PropTypes.bool, | ||
| onOpen: PropTypes.func, | ||
| onClose: PropTypes.func | ||
|
Contributor
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 think it makes sense to specify which props should be marked with
Contributor
Author
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 think just children may be the only required item do you see others?
Contributor
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 think you are correct. Also, I noticed that the Do we want to default the
Contributor
Author
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. good catch. i'm tempted to not use default propTypes and just let helix-ui pick the defaults. That way our wrapper defaults wont get in the way of what Helix may set as defaults in the future. (makes the wrapper slightly more light weight). @100stacks you have an any opinion on if we should be setting defaults props/attributes before passing them on to helix?
Member
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. @HelixDesignSystem/helix-react-core I think we left this open during our sync today. Balancing explicit defaults vs. implied HelixUI defaults. We can address again as needed. |
||
| }; | ||
|
|
||
| export default Alert; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| import React from 'react'; | ||
| import { storiesOf } from '@storybook/react'; | ||
| import { boolean, select, text } from '@storybook/addon-knobs/react'; | ||
| import Alert from './index'; | ||
|
|
||
| const TYPES = { | ||
| 'info': 'info', | ||
| 'error': 'error', | ||
| 'success': 'success', | ||
| 'warning': 'warning' | ||
| }; | ||
|
|
||
| storiesOf('Alert', module) | ||
| .add('All Knobs', () => { | ||
| let content = text('content', 'Nope! Nope! Nope! Nope! Nope!'); | ||
| let cta = text('cta', 'burn it'); | ||
| let status = text('status', 'spider'); | ||
| let persist = boolean('persist', false); | ||
| let type = select('type', TYPES, ''); | ||
|
|
||
| return ( | ||
| <Alert | ||
| { ...( cta && { cta }) } | ||
| { ...( status && { status }) } | ||
| { ...( persist && { persist }) } | ||
| { ...( type && { type }) } | ||
| >{content}</Alert> | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| /* Export helix-react definition */ | ||
| import Button from './Button'; | ||
| import Alert from './Alert'; | ||
| import Icon from './Icon'; | ||
|
|
||
| export default { | ||
| Button, | ||
| Icon, | ||
| Alert | ||
| }; |
Uh oh!
There was an error while loading. Please reload this page.