material-ui: [Pagination] nextButton won't disable after providing float 'count' props

  • The issue is present in the latest
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

When passing float ‘count’ props to PaginationComponent bugs occur. pagination

You can see our discussion with @oliviertassinari about this behavior in this link.

The ‘nextbutton’ is not disabled on the last page when count is provided with a float number.

<Pagination count={1.3} />

result: link to sandbox

Expected Behavior 🤔

PaginationComponent should return a warning when count is provided with a float number.

Steps to Reproduce 🕹

PaginationComponent proptypes uses number proptype for validating. Instead, I have changed to my custom proptype which warns the developer when he/she passes a float number as a proptype. Moreover, the component gives an error when it is not provided with a number at all.

Steps:

  1. Change PaginationComponent count proptype with custom proptype

My build result: codesandbox link Screenshot: Снимок экрана 2021-03-01 в 14 42 24

Context 🔦

I used the PaginationComponent whilst dividing the totalAmount into pages, of length 30. Sometimes this division resulted in float numbers which I passed to PaginationComponent thus I had a bug with the clickable next page button on the last page.

Your Environment 🌎

Using FireFox

`npx @material-ui/envinfo` ``` System: OS: macOS 11.2 Binaries: Node: 14.16.0 - /usr/local/bin/node Yarn: 1.22.10 - /usr/local/bin/yarn npm: 6.14.11 - /usr/local/bin/npm Browsers: Chrome: Not Found Edge: Not Found Firefox: 86.0 Safari: 14.0.3 npmPackages: @emotion/react: ^11.0.0 => 11.1.5 @emotion/styled: ^11.0.0 => 11.1.5 @material-ui/codemod: 5.0.0-alpha.24 @material-ui/core: 5.0.0-alpha.26 @material-ui/data-grid: 4.0.0-alpha.20 @material-ui/docs: 5.0.0-alpha.26 @material-ui/envinfo: 1.1.6 @material-ui/icons: 5.0.0-alpha.26 @material-ui/lab: 5.0.0-alpha.26 @material-ui/styled-engine: 5.0.0-alpha.25 @material-ui/styled-engine-sc: 5.0.0-alpha.25 @material-ui/styles: 5.0.0-alpha.26 @material-ui/system: 5.0.0-alpha.26 @material-ui/types: 5.1.7 @material-ui/unstyled: 5.0.0-alpha.26 @material-ui/utils: 5.0.0-alpha.26 @types/react: ^17.0.0 => 17.0.2 react: ^16.14.0 => 16.14.0 react-dom: ^16.14.0 => 16.14.0 styled-components: 5.2.1 typescript: ^4.1.2 => 4.2.2 ```

About this issue

  • Original URL
  • State: closed
  • Created 3 years ago
  • Reactions: 2
  • Comments: 16 (16 by maintainers)

Most upvoted comments

@fayzzzm Sounds like a plan. It might be simpler to wait for #25188 to be merged. You will need to changes.

@fayzzzm Perfect 👌

@oliviertassinari they use == not === and then in the scope of if they validate it again with ===:

if (props[propName] === null) {
  return new PropTypeError('The ' + location + ' `' + propFullName + '` is marked as required ' + ('in `' + componentName + '`, but its value is `null`.'));
}

In our case:

  • We just validate by ==, and then we don’t do anything. (You can correct me if am wrong).
  • We can easily pass null value which is incorrect.

Agree with the breakdown to smaller PRs’, will definitely do that.

propValue == null is important, we can’t remove it. Try without and should be able to understand why.

@fayzzzm Thanks for opening this issue. I agree, I think that we should add an integer prototype. This is a quick POC:

diff --git a/docs/pages/api-docs/pagination.json b/docs/pages/api-docs/pagination.json
index 79dd3cb717..33cada86a1 100644
--- a/docs/pages/api-docs/pagination.json
+++ b/docs/pages/api-docs/pagination.json
@@ -27,7 +27,7 @@
     },
     "showFirstButton": { "type": { "name": "bool" } },
     "showLastButton": { "type": { "name": "bool" } },
-    "siblingCount": { "type": { "name": "number" }, "default": "1" },
+    "siblingCount": { "type": { "name": "custom", "description": "integer" }, "default": "1" },
     "size": {
       "type": {
         "name": "enum",
diff --git a/docs/scripts/buildApi.ts b/docs/scripts/buildApi.ts
index 223a644258..4fb7c2cd6e 100644
--- a/docs/scripts/buildApi.ts
+++ b/docs/scripts/buildApi.ts
@@ -32,7 +32,8 @@ import { pageToTitle } from 'docs/src/modules/utils/helpers';
 import createGenerateClassName from '@material-ui/styles/createGenerateClassName';
 import getStylesCreator from '@material-ui/styles/getStylesCreator';
 import { createMuiTheme } from '@material-ui/core/styles';
-import { getLineFeed, getUnstyledFilename } from './helpers';
+import { getLineFeed, getUnstyledFilename } from 'docs/scripts/helpers';
+import generatePropTypeDescription from 'docs/src/modules/utils/generatePropTypeDescription';

 const DEMO_IGNORE = LANGUAGES_IN_PROGRESS.map((language) => `-${language}.md`);

@@ -126,83 +127,6 @@ function isElementAcceptingRefProp(type: PropTypeDescriptor): boolean {
   return /^elementAcceptingRef/.test(type.raw);
 }

-function generatePropTypeDescription(type: PropTypeDescriptor): string | undefined {
-  switch (type.name) {
-    case 'custom': {
-      if (isElementTypeAcceptingRefProp(type)) {
-        return `element type`;
-      }
-      if (isElementAcceptingRefProp(type)) {
-        return `element`;
-      }
-      if (isRefType(type)) {
-        return `ref`;
-      }
-      if (type.raw === 'HTMLElementType') {
-        return `HTML element`;
-      }
-
-      const deprecatedInfo = getDeprecatedInfo(type);
-      if (deprecatedInfo !== false) {
-        return generatePropTypeDescription({
-          // eslint-disable-next-line react/forbid-foreign-prop-types
-          name: deprecatedInfo.propTypes,
-        } as any);
-      }
-
-      const chained = getChained(type);
-      if (chained !== false) {
-        return generatePropTypeDescription(chained.type);
-      }
-
-      return type.raw;
-    }
-
-    case 'shape':
-      return `{ ${Object.keys(type.value)
-        .map((subValue) => {
-          const subType = type.value[subValue];
-          return `${subValue}${subType.required ? '' : '?'}: ${generatePropTypeDescription(
-            subType,
-          )}`;
-        })
-        .join(', ')} }`;
-
-    case 'union':
-      return (
-        type.value
-          .map((type2) => {
-            return generatePropTypeDescription(type2);
-          })
-          // Display one value per line as it's better for visibility.
-          .join('<br>&#124;&nbsp;')
-      );
-    case 'enum':
-      return (
-        type.value
-          .map((type2) => {
-            return escapeCell(type2.value);
-          })
-          // Display one value per line as it's better for visibility.
-          .join('<br>&#124;&nbsp;')
-      );
-
-    case 'arrayOf': {
-      return `Array&lt;${generatePropTypeDescription(type.value)}&gt;`;
-    }
-
-    case 'instanceOf': {
-      if (type.value.startsWith('typeof')) {
-        return /typeof (.*) ===/.exec(type.value)![1];
-      }
-      return type.value;
-    }
-
-    default:
-      return type.name;
-  }
-}
-
 /**
  * Returns `null` if the prop should be ignored.
  * Throws if it is invalid.
diff --git a/docs/src/modules/utils/generatePropTypeDescription.ts b/docs/src/modules/utils/generatePropTypeDescription.ts
index 28297de96f..7fabdbb3f1 100644
--- a/docs/src/modules/utils/generatePropTypeDescription.ts
+++ b/docs/src/modules/utils/generatePropTypeDescription.ts
@@ -60,6 +60,10 @@ function isRefType(type: PropTypeDescriptor): boolean {
   return type.raw === 'refType';
 }

+function isIntegerPropType(type: PropTypeDescriptor): boolean {
+  return type.raw === 'integerPropType';
+}
+
 function isElementAcceptingRefProp(type: PropTypeDescriptor): boolean {
   return /^elementAcceptingRef/.test(type.raw);
 }
@@ -76,6 +80,9 @@ export default function generatePropTypeDescription(type: PropTypeDescriptor): s
       if (isRefType(type)) {
         return `ref`;
       }
+      if (isIntegerPropType(type)) {
+        return 'integer';
+      }
       if (type.raw === 'HTMLElementType') {
         return `HTML element`;
       }
diff --git a/packages/material-ui-utils/src/index.ts b/packages/material-ui-utils/src/index.ts
index 7f4d526553..6e5d3c5466 100644
--- a/packages/material-ui-utils/src/index.ts
+++ b/packages/material-ui-utils/src/index.ts
@@ -6,6 +6,7 @@ export { default as exactProp } from './exactProp';
 export { default as formatMuiErrorMessage } from './formatMuiErrorMessage';
 export { default as getDisplayName } from './getDisplayName';
 export { default as HTMLElementType } from './HTMLElementType';
+export { default as integerPropType } from './integerPropType';
 export { default as ponyfillGlobal } from './ponyfillGlobal';
 export { default as refType } from './refType';
 export { default as unstable_capitalize } from './capitalize';
diff --git a/packages/material-ui-utils/src/integerPropType.js b/packages/material-ui-utils/src/integerPropType.js
new file mode 100644
index 0000000000..53306f8314
--- /dev/null
+++ b/packages/material-ui-utils/src/integerPropType.js
@@ -0,0 +1,34 @@
+
+// IE 11 support
+function ponyfillIsInteger(x) {
+  // eslint-disable-next-line no-restricted-globals
+  return typeof x === 'number' && isFinite(x) && Math.floor(x) === x;
+}
+
+const isInteger = Number.isInteger || ponyfillIsInteger;
+
+function requiredInteger(props, propName) {
+  const propValue = props[propName];
+
+  if (propValue == null || !isInteger(propValue)) {
+    return new RangeError(`\`${propName}\` must be an integer. Instead, \`${propValue}\` was provided.`);
+  }
+
+  return null;
+}
+
+export default function integerPropType(props, propName) {
+  if (process.env.NODE_ENV === 'production') {
+    return null;
+  }
+
+  const propValue = props[propName];
+
+  if (propValue == null) {
+    return null;
+  }
+
+  return requiredInteger(props, propName);
+}
+
+integerPropType.isRequired = requiredInteger;
diff --git a/packages/material-ui/src/Pagination/Pagination.js b/packages/material-ui/src/Pagination/Pagination.js
index aff728a648..c621a3c1ca 100644
--- a/packages/material-ui/src/Pagination/Pagination.js
+++ b/packages/material-ui/src/Pagination/Pagination.js
@@ -1,6 +1,7 @@
 import * as React from 'react';
 import PropTypes from 'prop-types';
 import clsx from 'clsx';
+import { integerPropType } from '@material-ui/utils';
 import { withStyles, useThemeVariants } from '../styles';
 import usePagination from '../usePagination';
 import PaginationItem from '../PaginationItem';
@@ -202,7 +203,7 @@ Pagination.propTypes = {
    * Number of always visible pages before and after the current page.
    * @default 1
    */
-  siblingCount: PropTypes.number,
+  siblingCount: /* @typescript-to-proptypes-ignore */ integerPropType,
   /**
    * The size of the component.
    * @default 'medium'
Screenshot 2021-03-02 at 12 49 40

While doing it, I have noticed that how we handle custom prop-types is inconsistent:

  • We could simplify the error messages. The component stack trace is often enough, we don’t need to mention the name of the component for instance
  • We have duplicated generatePropTypeDescription
  • We could always use PropType as a suffix in the name to remove any potential confusion.
  • We can leverage if (process.env.NODE_ENV === 'production') { to reduce the size of the custom prop-types in production.