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] POC: PickersTextField styling - outlined variant #10778

Merged
merged 28 commits into from
Nov 27, 2023

Conversation

noraleonte
Copy link
Contributor

@noraleonte noraleonte commented Oct 23, 2023

This PR is still work in progress

Part of #6384

Preview:

https://deploy-preview-10778--material-ui-x.netlify.app/x/react-date-pickers/date-field/

To do in scope of this PR

  • Add HelperText
  • Add adornments
  • Fix section sizing
  • check classes and slots consistency
  • Fix types

@noraleonte noraleonte added the component: pickers This is the name of the generic UI component, not the React module! label Oct 23, 2023
@mui-bot
Copy link

mui-bot commented Oct 23, 2023

Deploy preview: https://deploy-preview-10778--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 05b6540

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 23, 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 Oct 24, 2023
@noraleonte noraleonte self-assigned this Nov 2, 2023
@noraleonte noraleonte marked this pull request as ready for review November 2, 2023 14:40
@noraleonte
Copy link
Contributor Author

Opening this one up for review to start the conversation. I still have to remove the demo, comments and some simulated behavior, but I will still leave them hanging for a little while to make the review/testing easier.
I's also love a double check from @mui/design to make sure I didn't miss something and the new component is consistent with the material ui TextField.

@michelengelen michelengelen changed the base branch from master to next November 6, 2023 14:20
noraleonte and others added 5 commits November 22, 2023 14:03
Co-authored-by: Flavien DELANGLE <flaviendelangle@gmail.com>
Signed-off-by: Nora <72460825+noraleonte@users.noreply.github.com>
@noraleonte noraleonte changed the title [pickers] POC: Fake textfield styling - outlined [pickers] POC: PickersTextField styling - outlined variant Nov 24, 2023
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.

That's an awesome first step

Can I let you update your branch where you have my code to use the new FakeTextField and push it to Github so that I can take it from here on my PR?

@noraleonte noraleonte merged commit 7259384 into mui:next Nov 27, 2023
17 checks passed
@noraleonte
Copy link
Contributor Author

@flaviendelangle Thanks! Sure, I'll let you know when it's updated.

@noraleonte noraleonte deleted the fake-textfield-styling branch November 27, 2023 13:11
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

This looks great!
Thank you for the effort! 🙏

@@ -0,0 +1,84 @@
import * as React from 'react';
import { styled } from '@mui/material/styles';

Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: This looks like a copy of https://github.com/mui/material-ui/blob/master/packages/mui-material/src/OutlinedInput/NotchedOutline.js
If that's the case, maybe we could mention it? 🤔

notchedOutline: string;
/** Styles applied to the fake input element. */
input: string;
/** Styles applied to the section of the picker. */
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Isn't there something missing from this description? 🤔

error: string;
/** Styles applied to the NotchedOutline element. */
notchedOutline: string;
/** Styles applied to the fake input element. */
Copy link
Member

Choose a reason for hiding this comment

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

It could make sense to avoid the fake work here as well. 🤔
Maybe call it input sections container instead of fake input? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Discussed in #11258 where I'm renaming this class

firstInput.focus();
}

inputRef.current?.focus();
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Is it the correct location to call it?
If we need/should focus both the root element as well as optionally the first section, then maybe the order should be inverted? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Removed in #11258

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.

None yet

4 participants