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

[TextareaAutosize] Add a rowsMin prop #18314

Closed
1 task done
lcswillems opened this issue Nov 11, 2019 · 7 comments · Fixed by #18804
Closed
1 task done

[TextareaAutosize] Add a rowsMin prop #18314

lcswillems opened this issue Nov 11, 2019 · 7 comments · Fixed by #18804
Labels
component: text field This is the name of the generic UI component, not the React module! component: TextareaAutosize The React component. good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request

Comments

@lcswillems
Copy link
Contributor

lcswillems commented Nov 11, 2019

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

Add a autosize property to TextField to be used with the multiline prop.

With autosize, I would expect rows define the minimum number of rows.

Motivation 🔦

The TextareaAutosize had been released recently but the UI is ugly. It would be nice if it could be used with the TextField component. If it is already possible, I couldn't find any documentation about it.

@oliviertassinari

This comment has been minimized.

@lcswillems
Copy link
Contributor Author

lcswillems commented Nov 11, 2019

Okay, I see. It already uses it under the hood.

So, the only part relevant of my message is:

With autosize, I would expect rows define the minimum number of rows.

Or maybe only a minRows property would be sufficient.

@oliviertassinari oliviertassinari added component: text field This is the name of the generic UI component, not the React module! new feature New feature or request component: TextareaAutosize The React component. labels Nov 11, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 11, 2019

@lcswillems Thanks for the report. Something is not right. This logic breaks the consistency between the TextareaAutosize component and the InputBase:
https://github.com/mui-org/material-ui/blob/575776f3004c6ac655b128fbdb30bd4b35115ab7/packages/material-ui/src/InputBase/InputBase.js#L376-L378

For consistency, if we add a new prop, it should be named rowsMin, so it matches rowsMax (and probably rename it maxRows/minRows in v5).

Here are two different alternatives API:

I assume that in most cases, people will need to set a maximum number of rows (do you confirm?) However, we should aim for an intuitive API.


Ok, I think that I would support this change, I suspect it would be more intuitive to the many:

  • InputBase: add rowsMin, if rows is used => <textarea>, if rowsMax or rowsMin is used => TextareaAutosize
  • TextareaAutosize: add rowsMin, take precedence over rows. Hide / deprecate the rows prop for rowsMin instead.

Would you confirm?

@lcswillems
Copy link
Contributor Author

lcswillems commented Nov 11, 2019

rowsMin or minRows is the same for me, so rowsMin is better for consistency.

I don't know if what people uses most. Personally, I only needed rowsMin for the moment.

Yes, I confirm both points.

@oliviertassinari oliviertassinari changed the title Add a autosize property to TextField [TextField][TextareaAutosize] Add a rowsMin prop Nov 11, 2019
@oliviertassinari oliviertassinari added good first issue Great for first contributions. Enable to learn the contribution process. and removed help appreciated labels Dec 1, 2019
@oliviertassinari
Copy link
Member

I think that the diff would look something like this:

diff --git a/packages/material-ui/src/InputBase/InputBase.js b/packages/material-ui/src/InputBase/InputBase.js
index e425bd307..27446a3a9 100644
--- a/packages/material-ui/src/InputBase/InputBase.js
+++ b/packages/material-ui/src/InputBase/InputBase.js
@@ -191,6 +191,7 @@ const InputBase = React.forwardRef(function InputBase(props, ref) {
     renderSuffix,
     rows,
     rowsMax,
+    rowsMin,
     startAdornment,
     type = 'text',
     value: valueProp,
@@ -367,7 +368,7 @@ const InputBase = React.forwardRef(function InputBase(props, ref) {
       ref: null,
     };
   } else if (multiline) {
-    if (rows && !rowsMax) {
+    if (rows && !rowsMax && !rowsMin) {
       InputComponent = 'textarea';
     } else {
       inputProps = {
@@ -599,6 +600,10 @@ InputBase.propTypes = {
    * Maximum number of rows to display when multiline option is set to true.
    */
   rowsMax: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
+  /**
+   * Minimum number of rows to display when multiline option is set to true.
+   */
+  rowsMin: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
   /**
    * Start `InputAdornment` for this component.
    */
diff --git a/packages/material-ui/src/TextareaAutosize/TextareaAutosize.js b/packages/material-ui/src/TextareaAutosize/TextareaAutosize.js
index d7040ddc1..65dd103b3 100644
--- a/packages/material-ui/src/TextareaAutosize/TextareaAutosize.js
+++ b/packages/material-ui/src/TextareaAutosize/TextareaAutosize.js
@@ -27,7 +27,9 @@ const styles = {
 };

 const TextareaAutosize = React.forwardRef(function TextareaAutosize(props, ref) {
-  const { onChange, rows, rowsMax, style, value, ...other } = props;
+  const { onChange, rows, rowsMax, rowsMin: rowsMinProp = 1, style, value, ...other } = props;
+
+  const rowsMin = rowsMinProp || rows;

   const { current: isControlled } = React.useRef(value != null);
   const inputRef = React.useRef(null);
@@ -60,8 +62,8 @@ const TextareaAutosize = React.forwardRef(function TextareaAutosize(props, ref)
     // The height of the outer content
     let outerHeight = innerHeight;

-    if (rows != null) {
-      outerHeight = Math.max(Number(rows) * singleRowHeight, outerHeight);
+    if (rowsMin != null) {
+      outerHeight = Math.max(Number(rowsMin) * singleRowHeight, outerHeight);
     }
     if (rowsMax != null) {
       outerHeight = Math.min(Number(rowsMax) * singleRowHeight, outerHeight);
@@ -88,7 +90,7 @@ const TextareaAutosize = React.forwardRef(function TextareaAutosize(props, ref)

       return prevState;
     });
-  }, [rows, rowsMax, props.placeholder]);
+  }, [rowsMin, rowsMax, props.placeholder]);

   React.useEffect(() => {
     const handleResize = debounce(() => {
@@ -123,7 +125,7 @@ const TextareaAutosize = React.forwardRef(function TextareaAutosize(props, ref)
         onChange={handleChange}
         ref={handleRef}
         // Apply the rows prop to get a "correct" first SSR paint
-        rows={rows || 1}
+        rows={rowsMin}
         style={{
           height: state.outerHeightStyle,
           // Need a large enough different to allow scrolling.
@@ -159,13 +161,19 @@ TextareaAutosize.propTypes = {
    */
   placeholder: PropTypes.string,
   /**
-   * Minimum number of rows to display.
+   * Use `rowsMin` instead
+   *
+   * @deprecated
    */
   rows: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
   /**
    * Maximum number of rows to display.
    */
   rowsMax: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
+  /**
+   * Minimum number of rows to display.
+   */
+  rowsMin: PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
   /**
    * @ignore
    */

@oliviertassinari oliviertassinari changed the title [TextField][TextareaAutosize] Add a rowsMin prop [TextareaAutosize] Add a rowsMin prop Dec 1, 2019
@lcswillems
Copy link
Contributor Author

Yes, that looks good for me. I can do a PR for it if needed!

@oliviertassinari
Copy link
Member

@lcswillems This would be awesome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! component: TextareaAutosize The React component. good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants