-
Notifications
You must be signed in to change notification settings - Fork 106
feat(many): rework TextArea and dependent FormField components #2273
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
Conversation
6a10c93 to
1049f95
Compare
|
1049f95 to
e010ea4
Compare
0d24a0d to
03b756d
Compare
| css={invalid ? styles?.requiredAsterisk : {}} | ||
| aria-hidden={true} | ||
| > | ||
| {' '} | ||
| * |
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 logic rendering the asterisk has been moved to FormFieldLayout for the other components to here.
| {shouldShowIcon && ( | ||
| <span css={styles?.icon}> | ||
| {isErrorVariant ? ( | ||
| <AlertCircleInstUIIcon size="sm" color="errorColor" /> | ||
| ) : ( | ||
| <CheckCircle2InstUIIcon size="sm" color="successColor" /> | ||
| )} |
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.
now in the case of success and error (and newError), an icon is rendered
| const generateStyle = (componentTheme: RadioInputGroupTheme): RadioInputGroupStyle => { | ||
| const { invalidAsteriskColor } = componentTheme | ||
|
|
||
| return { | ||
| invalidAsterisk: { | ||
| color: invalidAsteriskColor |
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.
as the only theme variable invalidAsteriskColor has been removed, I removed the styles.ts too
| <span | ||
| css={this.props.styles?.textAreaOutline} | ||
| aria-hidden="true" | ||
| ref={(e) => { | ||
| this._highlightRef = e | ||
| }} | ||
| /> | ||
| ) : null} |
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.
As applying the focus outline was delegated to calcFocusOutlineStyles even when the TextArea is resized, this span was no loner needed was removed
| componentDidMount() { | ||
| this.myObserver = new ResizeObserver((entries) => { | ||
| for (const entry of entries) { | ||
| if (this._highlightRef) { | ||
| const entryStyle = window.getComputedStyle(entry.target) | ||
| this._highlightRef.style.transition = 'none' | ||
| this._highlightRef.style.width = `calc(${entryStyle.width} + 0.5rem)` | ||
| this._highlightRef.style.height = `calc(${entryStyle.height} + 0.5rem)` | ||
| clearTimeout(this.resizeTimeout) | ||
|
|
||
| this.resizeTimeout = setTimeout(() => { | ||
| if (this._highlightRef) { | ||
| this._highlightRef.style.transition = 'all 0.2s' | ||
| } | ||
| }, 500) | ||
| } | ||
| } | ||
| }) | ||
| }, []) | ||
|
|
||
| this.autoGrow() | ||
| this.props.makeStyles?.() | ||
| } |
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.
this logic was deleted as calcFocusOutlineStyles is used for focus outline even when the TextArea is getting resized
| textAreaOutline: { | ||
| label: 'textArea__outline', | ||
| pointerEvents: 'none', | ||
| position: 'absolute', | ||
| display: 'block', | ||
| boxSizing: 'border-box', | ||
| top: '-0.25rem', | ||
| bottom: '-0.25rem', | ||
| left: '-0.25rem', | ||
| right: '-0.25rem', | ||
| border: `${componentTheme.focusOutlineWidth} ${componentTheme.focusOutlineStyle} ${componentTheme.focusOutlineColor}`, | ||
| borderRadius: `calc(${componentTheme.borderRadius} * 1.5)`, | ||
| transition: 'all 0.2s', | ||
| // properties to transition on :focus | ||
| opacity: 0, | ||
| transform: 'scale(0.95)' |
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.
This is also not needed anymore as calcFocusOutlineStyles is used
matyasf
left a comment
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.
FYI: The focus ring does not animate because of a bug in the focus outline calculation, its fixed in the NumberInput PR.
Very nice work, see my comments for changes
03b756d to
87a2aa6
Compare
| **/ | ||
| @withDeterministicId() | ||
| @withStyle(generateStyle, generateComponentTheme) | ||
| class RadioInputGroup extends 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.
Why didn't you change this to functional 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.
@joyenjoyer I only changed those components to functional which needed their tokens rewired as a part of this PR and needed the useStyle hook to make it work. But as we agreed, the conversion to functional components is not a priority anymore.
| ) : ( | ||
| rawLabel | ||
| ) | ||
| const label = callRenderProp(renderLabel) |
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 may consider using React.createElement here instead of callRenderProp. We would like to deprecate it soon.
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.
hmm we should check that this is not a breaking change, e.g. make 100% sure that React.createElement can handle everything that callRenderProp can (e.g. just a string, a JSX, a React class 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.
@joyenjoyer I talked to @matyasf about this and he suggested to leave it as it is for now.
| ) : ( | ||
| rawLabel | ||
| ) | ||
| const label = callRenderProp(renderLabel) |
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 may consider React.createElement here instead of callRenderProp. We would like to deprecate it soon.
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.
@joyenjoyer see above
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 checked the design and went through the code. Very nice job! Left some minor comments/questions. I see that you changed some components to functional but not all of them. Why?
matyasf
left a comment
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.
nice work, I just found 1 issue
INSTUI-4812
ISSUE:
this PR:
TEST PLAN: