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

[docs] Fix /production-error crash #25839

Merged
merged 2 commits into from
Apr 20, 2021
Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Apr 19, 2021

Fixes a crash in /production-error/ (current next): https://deploy-preview-25839--material-ui.netlify.app/production-error/

The previous logic for creating the prev/next page navigation relied on the current order of pages. However, one could imagine that we later extend the pages to

const pages = [
  { pathname: '/an/existing/page' },
  { pathname: '/', ordered: false, disableDrawer: true },
  { pathname: 'https://medium.com/material-ui', title: 'Blog' },
  { pathname: '/a/new/page' }
];

Now /a/new/page would not be considered the next page after /an/existing/page. With this PR users will be able to navigate from /an/existing/page to /a/new/page with the next button.

The change required for this seems quite big but should be easier to follow and more robust against various positions of activePage. For example, activePage being the last entry in pages would lead to a crash. We didn't encounter this crash because of the current order of pages.

@eps1lon eps1lon added bug 🐛 Something doesn't work docs Improvements or additions to the documentation labels Apr 19, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Apr 19, 2021

No bundle size changes

Generated by 🚫 dangerJS against d244358

const [rating, setRating] = React.useState();
const [comment, setComment] = React.useState('');
const [commentOpen, setCommentOpen] = React.useState(false);
const [snackbarOpen, setSnackbarOpen] = React.useState(false);
const [snackbarMessage, setSnackbarMessage] = React.useState(false);
const inputRef = React.useRef();
const pageList = flattenPages(pages);
const currentPageNum = findIndex(pageList, (page) => page.pathname === activePage?.pathname);
const currentPage = pageList[currentPageNum];
Copy link
Member Author

Choose a reason for hiding this comment

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

If activePage was not null then currentPage === activePage i.e. currentPage was redundant.

* If a page should be excluded from this order, set `order: false`.
* You want to set `ordered: false` if you don't want the page to appear in an ordered list e.g. for previous/next page navigation.
*/
ordered?: boolean;
Copy link
Member Author

Choose a reason for hiding this comment

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

The wording is a bit rough. displayNav also works considering the disableDrawer wording. Maybe just keep the original word but keep the description?

@eps1lon eps1lon marked this pull request as ready for review April 19, 2021 12:49
@eps1lon eps1lon requested a review from mbrookes April 19, 2021 12:50
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

No regressions spotted

@eps1lon eps1lon merged commit f432a7a into mui:next Apr 20, 2021
@eps1lon eps1lon deleted the docs/fix-production-error branch April 20, 2021 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants