react-ui-org
Re-implement `Modal` component using HTMLDialogElement (#461)
PR updated:
* Introduced allowCloseOnBackdropClick and allowCloseOnEscapeKey prop, both set to true by default. It allows end-user to configure modal in such way that he/she can prevent closing by clicking on backdrop or pressing Escape key.
* Improved blocking of firing click events on primary and close button as there were missing conditions checking whether the button is disabled (it was there for a long time)
- The biggest problem was with propagation of events, Enter was mistakenly closing dialog in unwanted cases. I can talk about it on Monday if you wish.
While I introduces allowCloseOnBackdropClick and allowCloseOnEscapeKey, I am thinking about allowProceedOnEnterKey (or something like that) to disable Enter functionality which can be unwanted in some scenarios.
Just for info, tests need to be updated in https://github.com/react-ui-org/react-ui/pull/545 as new test environment is necessary.
Can @adamkudrna or anybody else help me with this?
I returned to this to complete the pull request but suddenly, I have all the texts inside of Modal in white even though there is no change in the code. master is OK. Does anybody know why? I do not want to rebased it until this is resolved.
Does anybody know why? I do not want to rebased it until this is resolved.
I added a fix. It seems to happen only when the docs is switched to dark mode.
I have rebased and squashed it. I'm gonna perform self-review first as I have seen that documentation is not up-to-date with latest changes and then I will request review from you-
Since rebase:
I fixed submitting dialog when form fields are present. It is not working either on master. Formerly, it was part of auto focus hook which I have modified. It was also containing focus trap that it can be removed now. Now, this functionality is part of dialogOnKeyDownHandler.js.
Then I have improved documentation and did minor fixes.
I made separate commit (https://github.com/react-ui-org/react-ui/pull/544/commits/2514fd74af700250c12546aa4f92952c56256c52) to demonstrate example with native form. Personally, I would not encourage this approach as every application differs in the way how it handles forms, state, validations and so. But it is true that it can be useful for example in that way, that native form will not be submitted until HTML validations are passing.
We had 4 examples of different kind, so I added native form as fifth and atypically explained it in the example. Even though it is atypical, I would leave it there as it is only place how we can describe example with modal with creating explicit section. And we do not want to handle all possibilities in the documentation, so it compromise I think.
@adamkudrna @mbohal, you can review the pull request and tell whether to include this native form example or drop it.
I fixed submitting dialog when form fields are present. It is not working either on master. Formerly, it was part of auto focus hook which I have modified. It was also containing focus trap that it can be removed now. Now, this functionality is part of dialogOnKeyDownHandler.js.
I just want to mention (it will be part of commit message) that even though HTML Dialog states that it automatically focuses first focusable element in dialog, it does not work as our solution. The problem is that it autofocuses only some elements and for example states when there are only buttons and no input fields do not work. So we have to keep this functionality and we are not able to remove it!
The only question is: Do we want to have autoFocus always enabled? Personally, I would suggest removing autoFocus prop and make it mandatory as native solution would automatically focuses some elements even when our autoFocus is off.
The only question is: Do we want to have autoFocus always enabled? Personally, I would suggest removing autoFocus prop and make it mandatory as native solution would automatically focuses some elements even when our autoFocus is off.
Agreed we want to make autoFocus always on.
After discussion with @mbohal, we decided not to change our autoFocus functionality. Even though native autofocus functionality was introduced into
I dug deep into in again @mbohal and I found out, that with autoFocus disabled (through props, not through deleting whole functionality), our autoFocus internally forces focus on