Skip to content
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

Remove react-autosize-textarea #10543

Merged
merged 6 commits into from
Sep 12, 2023

Conversation

notbakaneko
Copy link
Collaborator

react-autosize-textarea hasn't been updated in a while and doesn't have current versions of React in its dependencies.
It's just wrapping autosize, anyway, so we might as well use autosize directly - the component is just a convenience wrapper to attach and detach autosize and doesn't use forwardRef, making the implementation more straight-forward.

};

private lineHeight?: number;
private ref = this.props.innerRef ?? React.createRef<HTMLTextAreaElement>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be a getter instead so the value doesn't potentially get outdated when the props change? (although probably practically impossible? also there are a bunch of other ref-related things that's being set once in componentDidMount and assumed being the same in componentDidUpdate)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refs aren't managed by React other than committing a value to it after render, so messing with a ref wouldn't normally be triggering renders anyway

autoComplete='off'
className={classWithModifiers('chat-input__box', { disabled: this.inputDisabled })}
disabled={this.inputDisabled}
innerRef={this.inputBoxRef}
maxLength={channel?.messageLengthLimit ?? maxMessageLength}
maxRows={channel?.type === 'ANNOUNCE' ? 10 : 3}
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like the maximum rows option doesn't get applied correctly

const maxHeight = this.maxHeight;

return (
<textarea
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure exactly where but the styling seems to be slightly wrong

old

this pr

note the wrong background colour for the padding

it's slightly more interesting in chrome

Copy link
Collaborator Author

@notbakaneko notbakaneko Sep 11, 2023

Choose a reason for hiding this comment

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

The old one uses Math.round before multiplying the rows, this one applies Math.ceil after multiplying; current line-height is 17.5px

Copy link
Collaborator Author

@notbakaneko notbakaneko Sep 11, 2023

Choose a reason for hiding this comment

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

white box appears to be a change in autosize between 4 and the current version where previously it applied overflow: hidden scroll but now it doesn't 🤔

@nanaya nanaya merged commit ef1f6c0 into ppy:master Sep 12, 2023
3 checks passed
@notbakaneko notbakaneko deleted the remove-react-autosize-textbox branch September 13, 2023 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants