feat(ui5-dialog): introduce resizable property #2301
Conversation
There was a problem hiding this comment.
I have added a couple of comments. Overall looks good to me. One more thing should be added anyway. It is the min-height for the content of the dialog. As much as I can see from the spec it should be as follows:
min-height: 2.75remfor Cozymin-height: 2.5remfor Compact
packages/main/src/Dialog.js
Outdated
| onBeforeRendering() { | ||
| this.onPhone = isPhone(); | ||
| this.onDesktop = isDesktop(); | ||
| } | ||
|
|
||
| onEnterDOM() { | ||
| this._isRTL = this.effectiveDir === "rtl"; |
There was a problem hiding this comment.
I am not sure that this is the best place to set the RTL, because the direction can be changed multiple times after the component has entered the DOM. A better solution might be to put it in the onBeforeRendering method for example.
There was a problem hiding this comment.
Moved it to occur in onBeforeRendering. In the future, it might be better to do this check centrally (once) before all the ui5-webcomponents are rendered. I see a few web components are already doing this very same check, so it's fair to attempt to remove the duplication.
|
|
||
| this._initialX = event.clientX; | ||
| this._initialY = event.clientY; | ||
| this._initialWidth = Number.parseFloat(width); |
There was a problem hiding this comment.
Please use the global function parseFloat as it is natively supported by IE. This way babel would not have to transform it when it comes to the es5 build
There was a problem hiding this comment.
I have added a seperate pull request to add support for Number.parseFloat #2302
| @@ -80,6 +80,36 @@ describe("Dialog general interaction", () => { | |||
|
|
|||
| browser.keys("esc"); | |||
| }); | |||
|
|
|||
| it.only("resizable", () => { | |||
There was a problem hiding this comment.
it("resizable", () => {
Currently this is the only test that is being run from the whole page. After removing the keyword only, all the tests would be run. As much as I can see, the resizable dialog is initially opened. This might be a problem for the other tests. The best solution would be if all the dialogs are initially closed and in the test you open the needed dialog.
There was a problem hiding this comment.
Sorry, I forgot to remove this it.only. I also removed the setTimeout on Dialog.html to restore the other tests as well.
ilhan007
dismissed
fifoosid’s stale review
outdated - comments have been resolved
Part of #2082