EZEE-2660: 500 error occurs when publishing form in form on the fly #125
Conversation
| @@ -1062,6 +1062,7 @@ export default class UniversalDiscoveryModule extends Component { | |||
| onCancel={this.props.onCancel} | |||
| handlePublish={this.handlePublish} | |||
| restInfo={this.props.restInfo} | |||
| setMainContainerRef={this.setMainContainerRef} | |||
There was a problem hiding this comment.
I realised, after analysing ContentCreatorComponent, that we have here setMainContainerRef={ and not ref={. It was confusing at first. Maybe cleaner would be to wrap ContentCreatorComponent into forwardRef? But I'm not 100% sure. 🤔
There was a problem hiding this comment.
I didn't want to rewrite many files to follow the forwardRef convention. But, you're right in other cases I would go with forwardRef as it's a better approach. Here, I wanted to make it simple without to much rewriting.
There was a problem hiding this comment.
Later I also realised that, actually, we need two refs to be set - we are also missing setContentContainerRef. So, we even cannot use forwardRef as it only works for one reference.
| @@ -140,6 +172,9 @@ ContentCreatorComponent.propTypes = { | |||
| handlePublish: PropTypes.func.isRequired, | |||
| loadLocation: PropTypes.func, | |||
| restInfo: PropTypes.object.isRequired, | |||
| enablePublishButton: PropTypes.func.isRequired, | |||
There was a problem hiding this comment.
Are we passing this function (and other 2 below) via props? It seems to be unused inside component.
| iframeDoc.body.addEventListener('fbFormBuilderUnloaded', this.enablePublishButton, false); | ||
| } | ||
|
|
||
| enablePublishButton() { |
There was a problem hiding this comment.
Maybe we could merge enablePublishButton with disablePublishButton into togglePublishButton(isDisabled)? This might be cleaner. :)
There was a problem hiding this comment.
I made it on purpose. I wanted to have a clear path of function calls without a need to look at function/method params. Simpler for debugging in this case, when we are observing async actions caused by events.
src/modules/universal-discovery/components/content-creator/content.creator.component.js
Show resolved
Hide resolved
| @@ -11,23 +11,32 @@ export default class ContentCreatorComponent extends Component { | |||
|
|
|||
| this.handleIframeLoad = this.handleIframeLoad.bind(this); | |||
| this.handlePublish = this.handlePublish.bind(this); | |||
| this.enablePublishButton = this.enablePublishButton.bind(this); | |||
| this.disablePublishButton = this.disablePublishButton.bind(this); | |||
| this.renderPublishBtn = this.renderPublishBtn.bind(this); | |||
There was a problem hiding this comment.
There is no need to bind it to this, as far as I can see.
There was a problem hiding this comment.
I think we should remove this. 🙂
| type: 'button', | ||
| }; | ||
|
|
||
| if (this.state.publishBtnDisabled) { |
There was a problem hiding this comment.
For me, cleaner would be to add disabled: this.state.publishBtnDisabled to attrs above and remove if statement.
There was a problem hiding this comment.
No, I want to set attrs.disabled when needed. I don't want to pass attrs.disabled with undefined value.
There was a problem hiding this comment.
With approach that I suggested attrs.disabled would never be undefined.
| this.renderPublishBtn = this.renderPublishBtn.bind(this); | ||
|
|
||
| this._refIframe = null; | ||
| this._refContentContainer = null; |
There was a problem hiding this comment.
Actually, it seems to be unused.
| @@ -11,23 +11,32 @@ export default class ContentCreatorComponent extends Component { | |||
|
|
|||
| this.handleIframeLoad = this.handleIframeLoad.bind(this); | |||
| this.handlePublish = this.handlePublish.bind(this); | |||
| this.enablePublishButton = this.enablePublishButton.bind(this); | |||
| this.disablePublishButton = this.disablePublishButton.bind(this); | |||
| this.renderPublishBtn = this.renderPublishBtn.bind(this); | |||
There was a problem hiding this comment.
I think we should remove this. 🙂
sunpietro
force-pushed
the
ezee-2660-fix-cotf-with-form-builder
branch
from
c09785a
to
4dd7f64
Compare
|
@lserwatka the base branch has just been changed |
|
You can merge it up. |
|
Done |
https://jira.ez.no/browse/EZEE-2660
For QA
When the Form Builder app is opened inside the COTF modal, then the Publish button is disabled until an editor closes the Form Builder app.
Requires: https://github.com/ezsystems/ezplatform-form-builder/pull/129