Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
962115f
FP-990: Fix bug of info Message missing X button
wesleyboar Sep 27, 2021
b3e4bec
FP-990: Add some examples for SectionMessage usage
wesleyboar Sep 27, 2021
ef525f4
FP-990: <Message type=scope>, not <SectionMessage>
wesleyboar Sep 27, 2021
ff2a61e
FP-990: Fix failing ManageAccount test
wesleyboar Sep 27, 2021
17cb731
FP-990: (CSS) Separate SectionMessage form content
wesleyboar Sep 27, 2021
405a0c5
Merge branch 'main' into test/FP-990-section-message-not-alert-for-in…
wesleyboar Oct 4, 2021
5d7f652
Merge branch 'main' into test/FP-990-section-message-not-alert-for-in…
wesleyboar Mar 24, 2022
0cd1cbd
chore(lint): run prettier:fix
wesleyboar Mar 24, 2022
374f49e
Merge branch 'main' into test/FP-990-section-message-not-alert-for-in…
wesleyboar Apr 13, 2022
c9c5923
FP-990: Fix Ability to Dimiss SectionMessages
wesleyboar Apr 13, 2022
67df4c4
Quick: Fix IntroMessage Spacing
wesleyboar Apr 13, 2022
c180f22
Quick: In IntroMessage Use SectionMessage
wesleyboar Apr 13, 2022
75924aa
Quick: Update Message Components JSDocs
wesleyboar Apr 13, 2022
2dbf2c7
FP-990: More Fix Ability to Dimiss SectionMessages
wesleyboar Apr 13, 2022
175f619
Merge branch 'main' into test/FP-990-section-message-not-alert-for-in…
wesleyboar Apr 19, 2022
3078ed3
chore(prettier): "fix" whitespace
wesleyboar Apr 19, 2022
bc23e3b
FP-990: New Colors for Info Messages
wesleyboar Apr 20, 2022
2b55ebf
FP-1255: Fix bug that has not manifested
wesleyboar Apr 20, 2022
6a73188
Merge branch 'main' into test/FP-990-section-message-not-alert-for-in…
rstijerina Apr 25, 2022
8b015b5
fix(fp-990): use const not let
wesleyboar May 3, 2022
ce6795f
fix(fp-990): remove leftover setting of onDismiss
wesleyboar May 3, 2022
904765d
fix(fp-990): undefine props w/out new unused vars
wesleyboar May 3, 2022
44a3489
fix(fp-990): prettier fix
wesleyboar May 3, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ describe('Manage Account Page', () => {
expect(getByText(/Manage Account/)).toBeInTheDocument();
expect(getByText(/Back to Dashboard/)).toBeInTheDocument();
expect(getAllByText(/Loading.../)).toBeDefined();
expect(
getByRole('alert', { class: /introMessageGeneral/i })
).toBeInTheDocument();
expect(getByRole('status', { name: 'message' })).toBeInTheDocument();
Copy link
Member Author

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> is status.

});
});
8 changes: 4 additions & 4 deletions client/src/components/_common/InlineMessage/InlineMessage.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ import React from 'react';
import Message from '_common/Message';

/**
* Show an event-based message to the user
* @todo Document examples
* Show a component-specific event-based message to the user
* @example
* // Blah blah…
* <Sample jsx>
* // basic usage
* <InlineMessage type="success">Task complete.</InlineMessage>
* @see _common/Message
*/
const InlineMessage = (props) => {
// Override default props
Expand Down
31 changes: 20 additions & 11 deletions client/src/components/_common/IntroMessage/IntroMessage.jsx
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';
Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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 = {
Expand Down
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
Expand Up @@ -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);
Copy link
Member Author

@wesleyboar wesleyboar Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explanation. Same as for <ManageAccount> test update.

expect(getByText('Test Message')).not.toEqual(null);
});
});
Expand Down
28 changes: 25 additions & 3 deletions client/src/components/_common/Message/Message.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explanation. Changes about ARIA label (see ff2a61e) are related to fixing <ManageAccount> test case.

children,
className,
dataTestid,
Expand Down Expand Up @@ -123,6 +141,7 @@ const Message = ({
className={`${className} ${containerStyleNames}`}
role={role}
in={isVisible}
aria-label={ariaLabel}
data-testid={dataTestid}
>
<Icon
Expand Down Expand Up @@ -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) */
Expand All @@ -170,6 +191,7 @@ Message.propTypes = {
type: PropTypes.oneOf(TYPES).isRequired,
};
Message.defaultProps = {
ariaLabel: 'message',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

className: '',
canDismiss: false,
dataTestid: '',
Expand Down
10 changes: 6 additions & 4 deletions client/src/components/_common/Message/Message.module.scss
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

display: flex;
}
/* HELP: FP-1227: Why is this unset? */
p.is-scope-section {
margin-top: 0;
margin-bottom: 0;
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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);
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion client/src/components/_common/Section/SectionMessages.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directly for FP-1255.

{introMessageContent}
</IntroMessage>
);
Expand Down
40 changes: 25 additions & 15 deletions client/src/components/_common/SectionMessage/SectionMessage.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,38 +3,48 @@ 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) => {
Copy link
Member Author

Choose a reason for hiding this comment

The 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.onDismiss === undefined;
Comment on lines +14 to +15
Copy link
Member Author

@wesleyboar wesleyboar Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explanation. If users want a dismissible message—i.e. they passed canDismissbut choose to not manage that logic themselves—via isVisible and onDismissthen this component will manage that logic automatically.

P.S. Alternative ideas are welcome.


// Manage visibility
const onDismiss = useCallback(() => {
setIsVisible(!isVisible);
}, [isVisible]);
function onDismiss() {
if (autoManageVisible) {
setIsVisible(!isVisible);
}
if (!autoManageDismiss) {
props.onDismiss();
}
}

// Override default props
const messageProps = {
...Message.defaultProps,
...props,
isVisible,
onDismiss,
scope: 'section',
};
if (autoManageVisible) {
messageProps.isVisible = isVisible;
}
if (autoManageDismiss) {
messageProps.onDismiss = onDismiss;
}
Comment on lines +35 to +37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't line 30 set onDismiss regardless?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.propTypes = Message.propTypes;
SectionMessage.defaultProps = {
...Message.defaultProps,
isVisible: undefined,
onDismiss: undefined,
};
SectionMessage.defaultProps = Message.defaultProps;

export default SectionMessage;
5 changes: 3 additions & 2 deletions client/src/styles/settings/color.css
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note. I slowly replace --weak colors with "standardly-named" --(x-)light and --(x-)dark.

--global-color-success--normal: #43d130;
--global-color-success--weak: #43d13020;
--global-color-success--alt: var(--global-color-warning--normal);
Expand Down