fix(ui5-dialog): stretch content area of dialog #920

[gnseo]

No description provided.

[claassistantio]

All committers have signed the CLA.

[claassistantio]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[gnseo]

Hi, @MapTo0.

While I'm waiting for your review, I've added a new commit which is implementing new feature for adding focus event listener, and it is included into this PR.

Please consider this ASAP, this PR is quite urgent for us.
Thanks in advance.

[vladitasev]

Hello, @gnseo

Thank you for your interest in UI5 Web Components!

I'd like to ask you about both of your proposed changes:

1. What is the idea behind the Popup.css change? What do you want to achieve exactly? Is it that you're trying to stretch the dialog completely, because by design it should cover like 90% of the screen to still indicate there is a dialog (not 100%)
2. About the second change to Input.js. You can already listen for focusin and focusout on ui5-input without any changes. The new code that you added will never run, because the newly added _handleFocus method is never called (neigher by the template, nor by the js itself). Therefore it seems to do nothing.

Regards,
Vladi

[gnseo]

Hello, @vladitasev .

1. About the reason, please see the screenshot from the following link. Dialog: stretch is set to true, but the content of Dialog is not full stretched ui5-webcomponents-react#285 (comment)

2. I'm using @ui5/webcomponents-react and when I provide onFocus or onFocusIn to Input's props like below, it didn't work.

<Input onFocus={event=>{ console.log("on focus"); }} />

But after I add code as the commit in node_modules/@ui5/webcomponents/dist/Input.js, then it did work.

Thanks for your feedback, and I'm looking forward further feedback soon.

[fifoosid]

Hi @gnseo

1. I see your point and your PR is welcome.
2. In your application you are using the input from the ui5-webcomponents-react library, so you should open this issue there

If you only leave the changes about the dialog, we can totally merge this PR.

Best regards,
Filip

[gnseo]

Hi, @fifoosid .

About the Input, the change should be applied to @ui5/webcomponents, not to @ui5/webcomponents-react package.
After adding codes as my commit to node_modules/@ui5/webcomponents/dist/Input.js on local, the onFocus handler was working.

@MarcusNotheis : Could you double check this for me, please?

Regards.

[MarcusNotheis]

Hi @gnseo, @fifoosid,

I guess in this case the issue is rather with @ui5/webcomponents-react than with @ui5/webcomponents:
As you can see in the CodeSandbox the UI5 Web Component Input is firing this event but the for React version doesn't.

This is caused by the way we are binding events:
For Event Binding, we will grab a list of all events which are registered in the metadata of the respective UI5 Web Component. Thus adding the focus event to that list will enable us to bind that event.
@fifoosid: Is there a list of "standard HTML events" which are supported by all web components? Then you can basically join those lists. Or is it basically everything that starts with on.. in the HTMLElement.prototype?

[pskelin]

@fifoosid: Is there a list of "standard HTML events" which are supported by all web components? Then you can basically join those lists. Or is it basically everything that starts with on.. in the HTMLElement.prototype?

@MarcusNotheis I just checked the type definitions for the standard html elements in react. There is a DOMAttributes interface that lists all event handlers as well:
onFocus?: FocusEventHandler<T>;
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/f44f31789c08ce190cf58b0d441977dabb968f24/types/react/index.d.ts#L1316

Then they are used in the standard HTML element definitions like that
div: DetailedHTMLFactory<HTMLAttributes<HTMLDivElement>, HTMLDivElement>;
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/f44f31789c08ce190cf58b0d441977dabb968f24/types/react/index.d.ts#L2601

My guess would be that you need to indicate in the types of your components that you also extend/implement the same standard HTMLElement attributes.

[MarcusNotheis]

Hi all,
I just enhanced our withWebComponent wrapper to accept all unknown props as well (e.g. the onFocus event for the Input) and released this as @ui5/[email protected].
@gnseo Can you try with that version if you are now able to bind the onFocus event as expected?

Please be aware of the fact that all unknown props will not be wrapped in our Event object which provides functions like getParameter(). For these events the plain JS event class will be passed.

[gnseo]

Hi, all.

I've checked the event as @MarcusNotheis 's suggestion, and it worked.
But focus event might be supported by ui5-webcomponents in some time, if so, a runtime error would be occurred by the change since the argument send to event handler is changed.

So I've updated the withWebComponent, and created a pull request like below.
SAP/ui5-webcomponents-react#315

@MarcusNotheis Would you please review my update?

@fifoosid How about add focus to event metadata in this pull request to let Input support focus event?

[fifoosid]

Hi @gnseo

As @MarcusNotheis mentioned, he has added support for the focus event, so there is no need to add anything about focus handling in this repo.

You can leave only the changes about the dialog in this PR.

[gnseo]

Hi, @fifoosid .

I've removed focus from events metadata as your suggestion.
About events currently not supported by ui5-webcomponents, I would discuss with @MarcusNotheis on other PR.

Thanks.

[fifoosid]

Hi @gnseo

In order for the build to pass, you have to merge the master branch. After that I am going to merge this PR.