Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

[terra-clinical-header] Add support for hyperlink header title #923

Merged
merged 9 commits into from
Oct 9, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ Please review [Terra's Internationalization documentation](https://engineering.c
Contributing
</h2>

Please read through our [contributing guidelines](CONTRIBUTING.md). Included are directions for issue reporting and pull requests.
Please read through our [contributing guidelines](./CONTRIBUTING.md). Included are directions for issue reporting and pull requests.

<h2 id="local-development">
Local Development
Expand Down
3 changes: 3 additions & 0 deletions packages/terra-clinical-header/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

* Added
* Added `onTextClick` and support for hyperlink button header titles.

## 3.28.0 - (August 14, 2023)

* Changed
Expand Down
2 changes: 2 additions & 0 deletions packages/terra-clinical-header/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
"classnames": "^2.2.5",
"prop-types": "^15.5.8",
"terra-button": "^3.0.0",
"terra-enzyme-intl": "^3.4.0",
"terra-hyperlink": "^2.63.0",
"terra-theme-context": "^1.0.0"
},
"scripts": {
Expand Down
29 changes: 25 additions & 4 deletions packages/terra-clinical-header/src/Header.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link

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.

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.

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.

Copy link
Contributor Author

@trandrew1023 trandrew1023 Oct 9, 2023

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

Copy link

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.

Copy link
Contributor Author

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 3cddf3d

I 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

import ThemeContext from 'terra-theme-context';

import styles from './Header.module.scss';
Expand Down Expand Up @@ -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,
Copy link

Choose a reason for hiding this comment

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

Shouldn't this be "onClick"?

Copy link
Contributor Author

@trandrew1023 trandrew1023 Oct 9, 2023

Choose a reason for hiding this comment

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

I originally had this as onClick but was worried that would be confusing since there can be additional content added to the header by the consumer which could confuse what the onClick does. I can update this if removing the "text" portion is good with you still

Copy link

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@trandrew1023 trandrew1023 Oct 9, 2023

Choose a reason for hiding this comment

The 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 = {
Expand All @@ -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) {
Expand All @@ -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>
);
Expand Down
4 changes: 2 additions & 2 deletions packages/terra-clinical-header/src/Header.module.scss
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,14 @@
width: 100%;
word-wrap: break-word; /* For IE 10 and IE 11 */
}

/* stylelint-disable selector-max-compound-selectors */
.flex-end + .flex-fill {
.title {
padding-left: var(--terra-clinical-header-end-content-plus-title-padding-left, 0.35714rem);
}
}

.flex-fill + .flex-end {
.title {
padding-right: var(--terra-clinical-header-title-plus-end-content-padding-right, 0.35714rem);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import ContentHeader from '../example/ContentHeader?dev-site-example';
import HeaderLongText from '../example/HeaderLongText?dev-site-example';
import HeaderLongTextWithContent from '../example/HeaderLongTextWithContent?dev-site-example';
import Subheader from '../example/Subheader?dev-site-example';
import HyperlinkTitleHeader from '../example/HyperlinkTitleHeader?dev-site-example';

<Badge />

Expand All @@ -19,7 +20,7 @@ A Header component that allows elements to be placed on the left and right ends
- Install with [npmjs](https://www.npmjs.com):
- `npm install terra-clinical-header`

## Usage
## Usage

``` jsx
import Header from 'terra-clinical-header';
Expand All @@ -30,12 +31,13 @@ import Header from 'terra-clinical-header';
* [Responsive Support](https://engineering.cerner.com/terra-ui/about/terra-ui/component-standards#responsive-support)
* [Mobile Support](https://engineering.cerner.com/terra-ui/about/terra-ui/component-standards#mobile-support)

## Example
## Example
<TitleHeader title='Header With Title Only' />
<HyperlinkTitleHeader title='Header With Hyperlink Title' />
<ContentHeader title='Header With Content' />
<HeaderLongText title='Header With Long Title' />
<HeaderLongTextWithContent title='Header With Long Title, Children, And Content' />
<Subheader title='Subheader With Content' />

## Header Props Table
## Header Props Table
<HeaderPropsTable />
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;
95 changes: 82 additions & 13 deletions packages/terra-clinical-header/tests/jest/Header.test.jsx
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();
});

Loading
Loading