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

[fields] Select the first section instead of last when clicking right of content #9005

Merged
merged 4 commits into from May 16, 2023

Conversation

noraleonte
Copy link
Contributor

Fixes #8967

@noraleonte noraleonte added component: pickers This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature labels May 16, 2023
@mui-bot
Copy link

mui-bot commented May 16, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-9005--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 697.7 1,122 697.7 966.42 142.346
Sort 100k rows ms 752.1 1,341.6 1,288.9 1,135.94 209.282
Select 100k rows ms 245.4 415.7 316.7 311.24 63.133
Deselect 100k rows ms 191.7 302.6 216.7 227.98 39.5

Generated by 🚫 dangerJS against 5a4e1f8

@flaviendelangle
Copy link
Member

flaviendelangle commented May 16, 2023

Nice one !

For the failing test, I think we are just putting the selection on the last character (which a real person would not do).

    it('should set meridiem to AM when pressing "a" and a value is provided', () => {
      testFieldChange({
        format: adapter.formats.fullTime12h,
        defaultValue: adapter.date(new Date(2022, 5, 15, 14, 12, 25)),
        cursorPosition: 17,
        // Press "a"
        keyStrokes: [{ value: '02:12 a', expected: '02:12 AM' }],
      });
    });

Setting the cursor position to 16 should do the trick.
Note that we are running those test for each data library, so you only have to fix a few tests.

Side note, this cursorPosition is a pain (I don't want to count the index needed to select the "month" for example).
Now that we are exposing a ref that allows to select a section, we should be able to do something like:

    it('should set meridiem to AM when pressing "a" and a value is provided', () => {
      testFieldChange({
        // ...other params
        selectedSection: 'month',
      });
    });

That would be nicer, I'll try doing that later on.

Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

🥳 great first PR

@flaviendelangle flaviendelangle changed the title [fields] Fix selection of first element on DateField [fields] Select the first section instead of last when clicking right of content May 16, 2023
@noraleonte noraleonte merged commit f14010a into mui:master May 16, 2023
17 checks passed
@noraleonte
Copy link
Contributor Author

🥳 great first PR

Thanks! 🎉

@alexfauquette
Copy link
Member

It's a strike 🎳

This PR also solve #8945

@flaviendelangle
Copy link
Member

That's a good ROI PR 😆

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! enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fields] Should select 1st section instead of last when clicking on the right of the content
4 participants