-
Notifications
You must be signed in to change notification settings - Fork 50
[terra-clinical-header] Add support for hyperlink header title #923
Conversation
19c7e92
to
dcc64d3
Compare
/** | ||
* Callback function triggered via hyperlink button title. | ||
*/ | ||
onTextClick: PropTypes.func, |
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 this be "onClick"?
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 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
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 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 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!
@@ -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'; |
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 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
Functionally reviewed, great job @trandrew1023! |
Summary
What was changed:
onTextClick
prop to add callback function to the new hyperlink titleWhy it was changed:
Testing
This change was tested using:
Hyperlink triggered:
Jaws:
https://github.com/cerner/terra-clinical/assets/37750902/a67a2eb4-4093-4425-9c08-4c7a877046eb
Reviews
In addition to engineering reviews, this PR needs:
Additional Details
This PR resolves:
UXPLATFORM-9683
Thank you for contributing to Terra.
@cerner/terra