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

[pickers] Go to the default view when opening a picker #7484

Merged
merged 8 commits into from
Jan 18, 2023

Conversation

flaviendelangle
Copy link
Member

Part of #7440

We are moving back to v5 behavior on this

@flaviendelangle flaviendelangle added the component: pickers This is the name of the generic UI component, not the React module! label Jan 12, 2023
@flaviendelangle flaviendelangle self-assigned this Jan 12, 2023
@mui-bot
Copy link

mui-bot commented Jan 12, 2023

Messages
📖 Netlify deploy preview: https://deploy-preview-7484--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 686.6 1,210.8 1,210.8 972.64 233.543
Sort 100k rows ms 751.6 1,165.8 1,165.8 1,022.48 167.549
Select 100k rows ms 192.8 402.6 245.4 272.16 72.35
Deselect 100k rows ms 197.3 385.6 272.3 275.16 63.492

Generated by 🚫 dangerJS against a7d7409

@flaviendelangle flaviendelangle marked this pull request as ready for review January 12, 2023 09:11
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 16, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2023
Comment on lines 126 to 132
expect(handleViewChange.callCount).to.equal(1);

// Dismiss the picker
userEvent.keyPress(document.activeElement!, { key: 'Escape' });

openPicker({ type: 'date', variant: 'desktop' });
expect(handleViewChange.callCount).to.equal(2);
Copy link
Member

Choose a reason for hiding this comment

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

I do not have a full understanding of how the current view is managed. But this test raised a question:

Would it makes sense to switch back to the default view when we close the view. Such that when reopening, the current view will not be set and so we avoid getting two renderings:

  • one for opening
  • a second one for updating the view state

I'm wondering if removing the early return if(!open) in hook you modified would do the job

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it makes sense to switch back to the default view when we close the view.

That's an interesting question indeed
The view would probably not be reset if the user is controlling the opening state and closing it from the outside.
Not sure if this is problematic.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2023
@flaviendelangle flaviendelangle force-pushed the open-to-reset branch 2 times, most recently from f71d0ef to 42c904c Compare January 17, 2023 09:44
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2023
@flaviendelangle flaviendelangle merged commit 0171b6d into mui:next Jan 18, 2023
@flaviendelangle flaviendelangle deleted the open-to-reset branch January 18, 2023 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants