-
Notifications
You must be signed in to change notification settings - Fork 50
Conversation
.terraClinical-LabelValueView-label { | ||
color: $terra-grey-50; | ||
font-size: 1rem; | ||
line-height: 20px; |
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.
Recently we had a discussion to keep line-heights unit-less (dropping px / rem): https://css-tricks.com/almanac/properties/l/line-height/#article-header-id-0
This line-height value is also different than the one used below
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.
|
||
.terraClinical-LabelValueView-value { | ||
font-size: 1.4285714285714286rem; | ||
line-height: 1.4285714285714286rem; |
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.
Dave brought up a good point about this styling. If a String is passed in as a value we should style it based on UX feedback, but if a component was passed in, we probably shouldn't apply a font-size and line-height.
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.
The current implementation uses this UX styling based on the styling sheet for the TitleValueView. As for any children (say <h1>
) or components that are passed in, these styling are override. What is a better way to set these values?
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.
Could we dynamically set this class in the jsx file based on what children is? If it's a solitary string we add the class, otherwise not. I'm not 100% sold on this idea so I'd like other people to weigh in as well.
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 could do two separate props instead of children. valueText
and valueElement
or valueComponent
or something. I'd (personally) like to avoid type-checking props.
Or we could still use children too, something like:
<div {...customProps} className={labelValueViewClassNames}>
<div className="terraClinical-LabelValueView-label">{props.label}</div>
<div className="terraClinical-LabelValueView-value">{props.valueText}</div>
{children}
</div>
We'd just need to document that valueText will be styled a certain way, and that children could be used for more substantial components.
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'm cool with Tyler's suggestion. I definitely would like to see this addressed before it gets merged in.
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.
+1 To Tyler's suggestion of having a prop for value but keeping children, I would conditionally render the value wrapper.
The Button does something similar: https://github.com/cerner/terra-core/blob/master/packages/terra-button/src/Button.jsx#L73
When trying to render null nothing happens.
"main": "lib/LabelValueView.js", | ||
"private": true, | ||
"version": "0.0.0", | ||
"description": "terra-clinical-label-value-view", |
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.
A description should be added here, most people copy paste the description from the readme
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.
5e8c824
to
75323ee
Compare
/** | ||
* Child component(s) to display underneath the label. | ||
*/ | ||
children: PropTypes.element, |
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 would keep children as type node
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, I wasn't sure when I changed this. Changed Back Here
be4eaac
to
108af78
Compare
Summary
Implemented the react component LabelValueView to replace TitleValueCard.
Additional Details
The upgrade to React 15.5 is non-passive. So I locked react down at 15.4.2, as done in terra-core in this pull request.