-
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
FP-990 & FP-1255: Update <IntroMessage>
to Use Dismissible <SectionMessage>
#616
Changes from 17 commits
962115f
b3e4bec
ef525f4
ff2a61e
17cb731
405a0c5
5d7f652
0cd1cbd
374f49e
c9c5923
67df4c4
c180f22
75924aa
2dbf2c7
175f619
3078ed3
bc23e3b
2b55ebf
6a73188
8b015b5
ce6795f
904765d
44a3489
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 |
---|---|---|
@@ -1,8 +1,11 @@ | ||
import React from 'react'; | ||
import React, { useCallback, useState } from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import { Alert } from 'reactstrap'; | ||
import { useDispatch, useSelector } from 'react-redux'; | ||
|
||
import { SectionMessage } from '_common'; | ||
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. Changes on this page are for FP-990. |
||
|
||
import styles from './IntroMessage.module.css'; | ||
|
||
/** | ||
* Whether the name is of a known intro message | ||
* @param {String} messageName - The name of the message to check | ||
|
@@ -31,24 +34,30 @@ function IntroMessage({ children, className, messageName }) { | |
const dispatch = useDispatch(); | ||
const introMessages = useSelector((state) => state.introMessages); | ||
const shouldShow = isKnownMessage(messageName); | ||
const [isVisible, setIsVisible] = useState(shouldShow); | ||
|
||
function onDismiss(name) { | ||
// Manage visibility | ||
const onDismiss = useCallback(() => { | ||
const newMessagesState = { | ||
...introMessages, | ||
[name]: false, | ||
[messageName]: false, | ||
}; | ||
dispatch({ type: 'SAVE_INTRO', payload: newMessagesState }); | ||
} | ||
|
||
setIsVisible(!isVisible); | ||
}, [isVisible]); | ||
|
||
return ( | ||
<Alert | ||
isOpen={shouldShow} | ||
toggle={() => onDismiss(messageName)} | ||
color="secondary" | ||
className={className} | ||
<SectionMessage | ||
aria-label={messageName} | ||
type="info" | ||
canDismiss | ||
className={`${styles.root} ${className}`} | ||
isVisible={isVisible} | ||
onDismiss={onDismiss} | ||
> | ||
{children} | ||
</Alert> | ||
</SectionMessage> | ||
); | ||
} | ||
IntroMessage.propTypes = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
.root { | ||
/* To replicate margin that is unset by `<Message>` */ | ||
/* HACK: Overwrite spacing that was removed */ | ||
/* FP-1227: Standardize Message margin and do not hack here */ | ||
margin: var(--global-space--section-top) 0 !important; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,7 @@ describe('IntroMessage', () => { | |
); | ||
expect(container.getElementsByClassName('test-class').length).toEqual(1); | ||
// NOTE: The `status` role (https://www.w3.org/TR/html-aria/#index-aria-status) is more appropriate than the `alert` role (https://www.w3.org/TR/html-aria/#index-aria-alert), but setting the `role` attribute of an `Alert` is ineffectual | ||
expect(getByRole('alert')).not.toEqual(null); | ||
expect(getByRole('status')).not.toEqual(null); | ||
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. Explanation. Same as for |
||
expect(getByText('Test Message')).not.toEqual(null); | ||
}); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,12 +57,30 @@ export const DEFAULT_SCOPE = 'inline'; // FAQ: Historical support for default | |
|
||
/** | ||
* Show an event-based message to the user | ||
* @todo Document examples | ||
* @example | ||
* // Blah blah… | ||
* <Sample jsx> | ||
* // basic usage | ||
* <Message type="error" scope="inline">Invalid content.</Message> | ||
* @example | ||
* // manage dismissal and visibility | ||
* const [isVisible, setIsVisible] = useState(...); | ||
* | ||
* const onDismiss = useCallback(() => { | ||
* setIsVisible(!isVisible); | ||
* }, [isVisible]); | ||
* | ||
* return ( | ||
* <SectionMessage | ||
* type="warning" | ||
* isVisible={isVisible} | ||
* onDismiss={onDismiss} | ||
* > | ||
* Uh oh. | ||
* </SectionMessage> | ||
* ); | ||
* ... | ||
*/ | ||
const Message = ({ | ||
ariaLabel, | ||
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. Explanation. Changes about ARIA label (see ff2a61e) are related to fixing |
||
children, | ||
className, | ||
dataTestid, | ||
|
@@ -123,6 +141,7 @@ const Message = ({ | |
className={`${className} ${containerStyleNames}`} | ||
role={role} | ||
in={isVisible} | ||
aria-label={ariaLabel} | ||
data-testid={dataTestid} | ||
> | ||
<Icon | ||
|
@@ -151,6 +170,8 @@ const Message = ({ | |
); | ||
}; | ||
Message.propTypes = { | ||
/** How to label this message for accessibility (via `aria-label`) */ | ||
ariaLabel: PropTypes.string, | ||
/** Whether an action can be dismissed (requires scope equals `section`) */ | ||
canDismiss: PropTypes.bool, | ||
/** Message text (as child node) */ | ||
|
@@ -170,6 +191,7 @@ Message.propTypes = { | |
type: PropTypes.oneOf(TYPES).isRequired, | ||
}; | ||
Message.defaultProps = { | ||
ariaLabel: 'message', | ||
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. 👍 |
||
className: '', | ||
canDismiss: false, | ||
dataTestid: '', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
|
||
display: flex; | ||
} | ||
/* HELP: FP-1227: Why is this unset? */ | ||
p.is-scope-section { | ||
margin-top: 0; | ||
margin-bottom: 0; | ||
|
@@ -61,15 +62,15 @@ p.is-scope-section { | |
/* Modifiers: Type */ | ||
|
||
/* Design decided icon is not necessary for informational messages */ | ||
.is-info .icon { | ||
.is-info .icon:not(.close-icon) { | ||
Comment on lines
-64
to
+65
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. Explanation. Fix same problem as in #615 (comment), but using my proposed solution. |
||
display: none; | ||
} | ||
|
||
/* Modifiers: Scope */ | ||
|
||
.is-scope-inline { | ||
&.is-info .icon { | ||
color: var(--global-color-info--normal); | ||
color: var(--global-color-info--dark); | ||
} | ||
&.is-warn .icon { | ||
color: var(--global-color-warning--normal); | ||
|
@@ -94,10 +95,11 @@ p.is-scope-section { | |
|
||
/* Modifiers */ | ||
&.is-info { | ||
color: var(--global-color-info--dark); | ||
border-color: var(--global-color-info--normal); | ||
background-color: var(--global-color-info--weak); | ||
background-color: var(--global-color-info--light); | ||
& .type-icon { | ||
color: var(--global-color-info--normal); | ||
color: var(--global-color-info--dark); | ||
} | ||
} | ||
&.is-warn { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,7 +54,7 @@ function SectionMessages({ | |
const introMessageContent = introMessageText || MESSAGES[introMessageName]; | ||
const introMessage = introMessageContent && ( | ||
/* FAQ: Alternate message name allows tracking custom message dismissal */ | ||
<IntroMessage messageName={introMessageName || introMessageText}> | ||
<IntroMessage messageName={introMessageName || introMessageText} canDismiss> | ||
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. Directly for FP-1255. |
||
{introMessageContent} | ||
</IntroMessage> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,38 +3,46 @@ import PropTypes from 'prop-types'; | |
import Message from '_common/Message'; | ||
|
||
/** | ||
* Show an event-based message to the user | ||
* @todo Document examples | ||
* Show a section/page-specific event-based message to the user | ||
* @example | ||
* // Blah blah… | ||
* <Sample jsx> | ||
* // basic usage | ||
* <SectionMessage type="warning">Uh oh.</SectionMessage> | ||
* @see _common/Message | ||
*/ | ||
const SectionMessage = (props) => { | ||
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. Changes below on this page are for FP-1255. |
||
const [isVisible, setIsVisible] = useState(true); | ||
const autoManageVisible = props.canDismiss && props.isVisible === undefined; | ||
const autoManageDismiss = props.canDismiss && props.isVisible === undefined; | ||
|
||
// Manage visibility | ||
const onDismiss = useCallback(() => { | ||
setIsVisible(!isVisible); | ||
}, [isVisible]); | ||
function onDismiss() { | ||
if (autoManageVisible) { | ||
setIsVisible(!isVisible); | ||
} | ||
if (!autoManageDismiss) { | ||
props.onDismiss(); | ||
} | ||
} | ||
|
||
// Override default props | ||
const messageProps = { | ||
let messageProps = { | ||
wesleyboar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
...Message.defaultProps, | ||
...props, | ||
isVisible, | ||
onDismiss, | ||
onDismiss: onDismiss, | ||
scope: 'section', | ||
}; | ||
if (autoManageVisible) { | ||
messageProps.isVisible = isVisible; | ||
} | ||
if (autoManageDismiss) { | ||
messageProps.onDismiss = onDismiss; | ||
} | ||
Comment on lines
+35
to
+37
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. Doesn't line 30 set 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, nice catch. Line 30 is cruft. This conditional setting of the variable is desired. I dropped line 30 (ce6795f) and re-tested successfully. |
||
|
||
// Avoid manually syncing <Message>'s props | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
return <Message {...messageProps} />; | ||
}; | ||
SectionMessage.propTypes = { | ||
...Message.propTypes, | ||
isVisible: PropTypes.bool, | ||
onDismiss: PropTypes.func, | ||
}; | ||
SectionMessage.defaultProps = Message.defaultProps; | ||
SectionMessage.propTypes = Message.propTypes; | ||
let { isVisible, onDismiss, ...defaultProps } = Message.defaultProps; | ||
SectionMessage.defaultProps = defaultProps; | ||
wesleyboar marked this conversation as resolved.
Show resolved
Hide resolved
wesleyboar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
export default SectionMessage; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,8 +36,9 @@ | |
--global-color-accent--weak: #7754e840; /* =25% opacity */ | ||
/* on white, eq. to #DFD7FA */ | ||
--global-color-accent--alt: #d2cce7; | ||
--global-color-info--normal: var(--global-color-primary--x-dark); | ||
--global-color-info--weak: var(--global-color-primary--x-light); | ||
--global-color-info--dark: var(--global-color-primary--x-dark); | ||
--global-color-info--normal: var(--global-color-primary--normal); | ||
--global-color-info--light: var(--global-color-primary--x-light); | ||
Comment on lines
-43
to
+45
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. Note. I slowly replace |
||
--global-color-success--normal: #43d130; | ||
--global-color-success--weak: #43d13020; | ||
--global-color-success--alt: var(--global-color-warning--normal); | ||
|
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.
Explanation. Update test for
<IntroMessage>
, which has different aria role than did<Alert>
. The aria role for any<(...)Message>
isstatus
.