-
Notifications
You must be signed in to change notification settings - Fork 305
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
feat(content-explorer-modal-container): add optional info notice #3634
feat(content-explorer-modal-container): add optional info notice #3634
Conversation
8399665
to
0e54865
Compare
@@ -552,6 +559,7 @@ class ContentExplorer extends Component { | |||
}} | |||
{...contentExplorerProps} | |||
> | |||
{isInfoNoticeVisible && <ContentExplorerInfoNotice infoNoticeText={infoNoticeText} />} |
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.
you can use one prop to control this instead of two:
{isInfoNoticeVisible && <ContentExplorerInfoNotice infoNoticeText={infoNoticeText} />} | |
{infoNoticeText && <ContentExplorerInfoNotice infoNoticeText={infoNoticeText} />} |
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.
Updated
...nt-explorer/content-explorer-modal-container/__tests__/ContentExplorerModalContainer.test.js
Show resolved
Hide resolved
import InlineNotice from '../../../components/inline-notice'; | ||
|
||
const ContentExplorerInfoNotice = ({ infoNoticeText }) => ( | ||
<InlineNotice type="info" className="content-explorer-info-notice"> |
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.
should this use the InlineNotice from blueprint?
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.
Right, updated. Now the whole implementation is simpler
import ContentExplorerInfoNotice from '../ContentExplorerInfoNotice'; | ||
|
||
describe('features/content-explorer/content-explorer/ContentExplorerInfoNotice', () => { | ||
const renderComponent = infoNoticeText => shallow(<ContentExplorerInfoNotice infoNoticeText={infoNoticeText} />); |
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.
new tests should use react testing library
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.
Updated
97668e2
to
5f31508
Compare
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.
Looks good!
@@ -0,0 +1,25 @@ | |||
import * as React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import { injectIntl } from 'react-intl'; |
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.
we've been moving towards useIntl
now
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.
Updated
<InlineNotice | ||
variant="info" | ||
variantIconAriaLabel={intl.formatMessage(messages.infoNoticeIconAriaLabel)} | ||
className="content-explorer-info-notice" |
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.
we don't need this class anymore
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.
Updated
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.
we've been writing new files in tsx
although maybe you can write this inline of ContentExplorer since it doesn't need to be a separate component?
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.
Updated the files to .tsx
. I tried inlining it, but came across some issues with injecting i18n in tests, so I decided to simply update to .tsx
...nt-explorer/content-explorer-modal-container/__tests__/ContentExplorerModalContainer.test.js
Show resolved
Hide resolved
ec77f18
to
6b386e8
Compare
This PR adds an optional informational notice into the Content Explorer Modal Container. By default the notice is not shown. The notice text is passed in
infoNoticeText
. The text passed may not be empty for the notice to be shown.