-
Notifications
You must be signed in to change notification settings - Fork 50
[terra-clinical-header] Add support for hyperlink header title #923
Changes from 7 commits
f3cf551
045e9af
b8510e8
96948b2
dcc64d3
5c19471
84b23f4
3cddf3d
132963b
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 |
---|---|---|
|
@@ -2,6 +2,8 @@ import React, { useContext } from 'react'; | |
import PropTypes from 'prop-types'; | ||
import classNames from 'classnames'; | ||
import classNamesBind from 'classnames/bind'; | ||
|
||
import HyperlinkButton from 'terra-hyperlink'; | ||
import ThemeContext from 'terra-theme-context'; | ||
|
||
import styles from './Header.module.scss'; | ||
|
@@ -59,6 +61,11 @@ const propTypes = { | |
* A Boolean indicating if element is a subheader. | ||
*/ | ||
isSubheader: PropTypes.bool, | ||
|
||
/** | ||
* Callback function triggered via hyperlink button title. | ||
*/ | ||
onTextClick: PropTypes.func, | ||
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. Shouldn't this be "onClick"? 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 originally had this as 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 will be clear. We are passing to the onClick of the Hyperlink component. It would be consistent. 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. Sounds good, got that updated here and also added a bit more documentation for the prop 132963b Thank you! |
||
}; | ||
|
||
const defaultProps = { | ||
|
@@ -70,7 +77,16 @@ const defaultProps = { | |
}; | ||
|
||
const Header = ({ | ||
children, title, startContent, endContent, text, level, id, isSubheader, ...customProps | ||
children, | ||
title, | ||
startContent, | ||
endContent, | ||
text, | ||
level, | ||
id, | ||
isSubheader, | ||
onTextClick, | ||
...customProps | ||
}) => { | ||
const theme = useContext(ThemeContext); | ||
if (title) { | ||
|
@@ -85,12 +101,17 @@ const Header = ({ | |
} | ||
|
||
let titleElement; | ||
if (title || text) { | ||
const HeaderElement = (level) ? `h${level}` : 'h1'; | ||
const titleContent = title || text; | ||
if (titleContent) { | ||
const HeaderElement = level ? `h${level}` : 'h1'; | ||
titleElement = ( | ||
<div className={cx('title-container')}> | ||
<HeaderElement id={id} className={cx('title')}> | ||
{title || text} | ||
{onTextClick ? ( | ||
<HyperlinkButton onClick={onTextClick} text={titleContent} /> | ||
) : ( | ||
titleContent | ||
)} | ||
</HeaderElement> | ||
</div> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import React from 'react'; | ||
import Header from 'terra-clinical-header'; | ||
|
||
const HyperlinkTitleHeader = () => ( | ||
// eslint-disable-next-line no-console | ||
<Header onTextClick={() => { console.log('Hyperlink title clicked'); }} text="John Smith" level={3} /> | ||
); | ||
|
||
export default HyperlinkTitleHeader; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
import React from 'react'; | ||
|
||
import Header from '../../../Header'; | ||
|
||
const HyperlinkTitleHeader = () => ( | ||
<Header onTextClick={() => {}} text="John Smith" /> | ||
); | ||
|
||
export default HyperlinkTitleHeader; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,68 +1,137 @@ | ||
import React from 'react'; | ||
import { shallowWithIntl } from 'terra-enzyme-intl'; | ||
|
||
import Header from '../../src/Header'; | ||
|
||
const mockFunc = jest.fn(); | ||
afterEach(() => { | ||
// restore the spy created with spyOn | ||
jest.restoreAllMocks(); | ||
}); | ||
|
||
it('should render a default component', () => { | ||
const header = render(<Header />); | ||
const header = shallow(<Header />); | ||
expect(header).toMatchSnapshot(); | ||
}); | ||
|
||
it('should render a header with title', () => { | ||
const header = shallow(<Header text="title" />); | ||
|
||
const headerTitle = header.find('h1'); | ||
expect(headerTitle.text()).toEqual('title'); | ||
expect(header).toMatchSnapshot(); | ||
}); | ||
|
||
it('should render a header with title and heading level', () => { | ||
const header = render(<Header id="test-id" level={1} text="title" />); | ||
it('should render a header with id', () => { | ||
const header = shallow(<Header id="test-id" text="title" />); | ||
|
||
const headerTitle = header.find('h1'); | ||
expect(headerTitle.prop('id')).toEqual('test-id'); | ||
expect(headerTitle.text()).toEqual('title'); | ||
expect(header).toMatchSnapshot(); | ||
}); | ||
|
||
it('should render a header with heading level', () => { | ||
const header = shallow(<Header level={2} text="title" />); | ||
|
||
const headerTitle = header.find('h2'); | ||
expect(headerTitle.text()).toEqual('title'); | ||
expect(header).toMatchSnapshot(); | ||
}); | ||
|
||
it('should render a header with content on the left', () => { | ||
const header = render(<Header level={1} startContent={<div>start content</div>} />); | ||
const startContent = <div id="start-id">start content</div>; | ||
const flexFill = <div className="flex-fill" />; | ||
const flexEnd = <div className="flex-end">{startContent}</div>; | ||
const header = shallow(<Header startContent={startContent} />); | ||
|
||
// ensure flex-fill title container is after start content | ||
expect(header.find('.flex-header').props().children[0]).toEqual(flexEnd); | ||
expect(header.find('.flex-header').props().children[1]).toEqual(flexFill); | ||
expect(header).toMatchSnapshot(); | ||
}); | ||
|
||
it('should render a header with content on the right', () => { | ||
const header = render(<Header level={1} endContent={<div>end content</div>} />); | ||
const endContent = <div id="end-id">end content</div>; | ||
const flexFill = <div className="flex-fill" />; | ||
const flexEnd = <div className="flex-end">{endContent}</div>; | ||
const header = shallow(<Header endContent={endContent} />); | ||
|
||
// ensure flex-fill title container is before end content | ||
expect(header.find('.flex-header').props().children[1]).toEqual(flexFill); | ||
expect(header.find('.flex-header').props().children[3]).toEqual(flexEnd); | ||
expect(header).toMatchSnapshot(); | ||
}); | ||
|
||
it('should render a header with all content', () => { | ||
const header = render(( | ||
const startContent = <div id="start-id">start content</div>; | ||
const endContent = <div id="end-id">end content</div>; | ||
const flexFill = ( | ||
<div className="flex-fill"> | ||
<div className="title-container"> | ||
<h1 className="title">Title</h1> | ||
</div> | ||
</div> | ||
); | ||
const flexEndStart = <div className="flex-end">{startContent}</div>; | ||
const flexEndEnd = <div className="flex-end">{endContent}</div>; | ||
const header = shallow(( | ||
<Header | ||
startContent={<div>start content</div>} | ||
startContent={startContent} | ||
text="Title" | ||
endContent={<div>end content</div>} | ||
level={1} | ||
endContent={endContent} | ||
/> | ||
)); | ||
|
||
const headerTitle = header.find('h1'); | ||
expect(headerTitle.text()).toEqual('Title'); | ||
expect(header.find('.flex-header').props().children[0]).toEqual(flexEndStart); | ||
expect(header.find('.flex-header').props().children[1]).toEqual(flexFill); | ||
expect(header.find('.flex-header').props().children[3]).toEqual(flexEndEnd); | ||
expect(header).toMatchSnapshot(); | ||
}); | ||
|
||
it('should render a subheader with title and heading level', () => { | ||
const consoleSpy = jest.spyOn(global.console, 'warn'); | ||
const subheader = render(<Header level={1} title="title" isSubheader />); | ||
const subheader = shallow(<Header title="title" isSubheader />); | ||
const titleWarningMessage = 'The `title` prop has been renamed to `text`. Please update all references of `title` prop to `text`.'; | ||
|
||
expect(subheader.prop('className')).toEqual('flex-header subheader'); | ||
expect(subheader.find('h1').text()).toEqual('title'); | ||
expect(consoleSpy).toHaveBeenCalledWith(titleWarningMessage); | ||
expect(subheader).toMatchSnapshot(); | ||
}); | ||
|
||
it('should render a subheader with all content', () => { | ||
const subheader = render(( | ||
const subheader = shallow(( | ||
<Header | ||
startContent={<div>start content</div>} | ||
text="Title" | ||
endContent={<div>end content</div>} | ||
isSubheader | ||
level={1} | ||
/> | ||
)); | ||
expect(subheader).toMatchSnapshot(); | ||
}); | ||
|
||
it('should render a header with default heading level when level not set', () => { | ||
const consoleSpy = jest.spyOn(global.console, 'warn'); | ||
const header = render(<Header text="This title should render with a default level" />); | ||
const title = 'This title should render with a default level'; | ||
const header = shallow(<Header text={title} />); | ||
const levelWarningMessage = 'Default heading level may not appropriate has it would fail to convey context of heading in a site / application where it is used. Heading level should be set explicitly depending on the position of header in site / application to allow screen readers to identify headers consistently.'; | ||
|
||
expect(header.find('h1').text()).toEqual(title); | ||
expect(consoleSpy).toHaveBeenCalledWith(levelWarningMessage); | ||
expect(header).toMatchSnapshot(); | ||
}); | ||
|
||
it('should render a header with hyperlink title', () => { | ||
const header = shallowWithIntl(<Header onTextClick={mockFunc} text="Title" />); | ||
|
||
expect(header.find('h1').length).toEqual(1); | ||
const hyperlinkButton = header.find('InjectIntl(Hyperlink)'); | ||
expect(hyperlinkButton.prop('onClick')).toEqual(mockFunc); | ||
expect(hyperlinkButton.prop('text')).toEqual('Title'); | ||
expect(header).toMatchSnapshot(); | ||
}); | ||
|
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.
Shouldn't we use Hyperlink instead of HyperlinkButton since that is the name of the component. It makes troubleshooting more difficult if a different name is used.
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.
@cm9361 and all, the "Hyperlink Button" is technically made of a button but is programmatically understood as a link (required for screen reader users). The best practice is to always make links using a proper hyperlink (Terra Hyperlink). However, there are a few instances where a Hyperlink Button may be required instead of a link due to the need for code to use a button (purely technical implementation requirement because a link cannot be used). I cannot speak to whether this use case is 100% required to use a Hyperlink Button, but from an accessibility perspective, this would pass the accessibility requirements.
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.
@trandrew1023 I've reviewed the implementation and it looks good. I'll update the labels on the page to reflect that I've reviewed.
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 can also use Hyperlink instead and everything is still the same as long as onClick is passed to it. The reason for using HyperlinkButton however was because of the example on the Hyperlink doc for Hyperlink Button also uses HyperlinkButton when onClick prop is used
https://engineering.cerner.com/terra-ui/components/cerner-terra-core-docs/hyperlink/about
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 made this comment because the actual name of the component is Hyperlink. In the examples provided, the component is imported as "Hyperlink". As a developer, I would try to find a HyperlinkButton package if I saw this without reviewing the import statement.
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.
Yeah that's a good catch, the example for the hyperlink button with the
onClick
had that import, but good reminder to be more cautious especially after our discussion today on the .isRequired prop earlier not to take things at face value. Updated here 3cddf3dI think it would be good to update the example in since that can cause confusion for others. I can create that PR since it's quick instead of creating a Jira https://github.com/cerner/terra-core/blob/94880b5c38757f285c68ddb373c86ad0363af45b/packages/terra-core-docs/src/terra-dev-site/doc/hyperlink/example/DefaultHyperlinkButton.jsx#L3