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] Support empty sections #10307

Merged
merged 9 commits into from
Dec 11, 2023
Merged

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Sep 11, 2023

Explore #10279

@LukasTy I would like to have your opinion on this one.
It's clearly pretty niche.

If we want to support it, I'll add some tests.

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

mui-bot commented Sep 11, 2023

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

Generated by 🚫 dangerJS against c146c31

@flaviendelangle flaviendelangle force-pushed the empty-section branch 2 times, most recently from dbe2ea0 to 603ba80 Compare September 11, 2023 15:38
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 like an interesting/niche addition, but it could prove useful in certain scenarios.
It does not seem to cause too much headache code-wise, hence, I'm not against introducing it. 👌

I've explored a solution along the lines of the issue authors case and it seems to work well.
The only issue I could see is that after the value is set and you try to change it using the keyboard - the value get's reset.

Have you explored the option of having such support when selectedSections is undefined or null, it could avoid the extra head-ache of having an empty section. 🤔

@flaviendelangle
Copy link
Member Author

I've explored a solution along the lines of the issue authors case and it seems to work well.

In your example I can't edit the value, it's throws invalid dates, not sure why.

Have you explored the option of having such support when selectedSections is undefined or null, it could avoid the extra head-ache of having an empty section

For me the problem is that if we have 0 section (which is the case before this PR), then the whole escaped format is not put in any section as a start or end separator (which is the only way I see to render it).

@LukasTy LukasTy added enhancement This is not a bug, nor a new feature feature: Keyboard editing Related to the pickers keyboard edition labels Sep 14, 2023
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.

In your example I can't edit the value, it's throws invalid dates, not sure why.

With which editing are you having problems?
You can easily edit with the picker and I expect that this is the behavior the issue author is expecting.

The main question is—how do we handle certain editing actions when an empty section is selected?
Should we simply disallow all the actions and call it a day? 🤔 😆

@flaviendelangle
Copy link
Member Author

Screencast.2023-09-14.14.16.23.mp4

@LukasTy
Copy link
Member

LukasTy commented Sep 14, 2023

My guess was that this is something that the issue author expected as a final result. 🤔
But I could be mistaken... 🙈

Screen.Recording.2023-09-14.at.15.29.40.mov

@flaviendelangle
Copy link
Member Author

I'd be surprised.
Once the value is filled, the behavior is good IMHO
But when there is no value and you try to do a field editing, I don't think it's ever a good option to display "Invalid Date".

@LukasTy
Copy link
Member

LukasTy commented Sep 14, 2023

Once the value is filled, the behavior is good IMHO

It's not fine if you try modifying the value in any way (i.e. Arrows).

But when there is no value and you try to do a field editing, I don't think it's ever a good option to display "Invalid Date".

It wasn't the best and most clear example. I've replaced the empty format in the mentioned demo from an incorrect null to [Do] - [Do].
Maybe now the behavior is more aligned between my expectation, your expectation, and a presumable issue author's expectation? 🤔

@flaviendelangle
Copy link
Member Author

I think this matches exactly what I am building internally
So for me we could start by proposing your solution indded 👍

@flaviendelangle
Copy link
Member Author

Closing because the user-land fix proposed by @LukasTy works well

@LukasTy
Copy link
Member

LukasTy commented Oct 26, 2023

Closing because the user-land fix proposed by @LukasTy works well

@flaviendelangle my suggested example is based on the changes in this PR, without them such behavior doesn't work.
In other words, this PR is still relevant, especially when we see more interest in it. 🤔

@flaviendelangle
Copy link
Member Author

Oh sorry 😬
Do you need all the changes of this PR ?

@LukasTy
Copy link
Member

LukasTy commented Oct 26, 2023

Do you need all the changes of this PR?

Possibly. Didn't try going the removal route and checking what would be the minimal working scenario, but the example basically relies on the empty section support. 🤔

@flaviendelangle
Copy link
Member Author

I'll re-open a PR then 👍

@flaviendelangle
Copy link
Member Author

Test added

@flaviendelangle flaviendelangle marked this pull request as ready for review November 10, 2023 08:29
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.

The addition LGTM. 👍
We could consider adding a demo about such usage, but on the other hand, it can be done when we see at least a second request for something like this, because the situation that needs this seems quite niche. 🤔

@flaviendelangle
Copy link
Member Author

I agree that we can wait a little before adding a doc example 👍

@flaviendelangle flaviendelangle merged commit 1d7a464 into mui:next Dec 11, 2023
4 checks passed
@flaviendelangle flaviendelangle deleted the empty-section branch December 11, 2023 09:19
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 feature: Keyboard editing Related to the pickers keyboard edition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants