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

Create namespace dialog bugfix (production) #167

Merged
merged 1 commit into from
Dec 14, 2015

Conversation

taimir
Copy link
Contributor

@taimir taimir commented Dec 11, 2015

This is not meant to be merged immediately.

There is currently a bug when running 'gulp serve:prod' - the namespace creation dialog in the deploy view does not respond (the dialog window does not open). Now looking at it, this was because of the following two issues:

  • Some of the referenced functions were actually not @exported, and hence their names were minified => they couldn't be found after compilation in production. I've added the respective @export annotations.
  • After fixing the first issue, another problem appeared: apparently the $mdDialog.show() method has problems resolving the template url '/deploy/createnamespace.html' when the app is run in production mode.

Pasting the template directly as shown here works, and this is a quick-fix workaround. Nevertheless, could anyone propose a better method of handling this?

@taimir
Copy link
Contributor Author

taimir commented Dec 11, 2015

@@ -28,7 +28,23 @@ export default function showNamespaceDialog(mdDialog, event, namespaces) {
controllerAs: 'ctrl',
clickOutsideToClose: true,
targetEvent: event,
templateUrl: '/deploy/createnamespace.html',
template: `<md-dialog aria-label="Create a new namespace" layout="column" layout-padding>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave the html template file and just change /deploy/createnamespace.html to deploy/createnamespace.html. This should work.

@floreks
Copy link
Member

floreks commented Dec 11, 2015

Good catch. Just add @export where it's missing and change template path. Everything should work.

@taimir
Copy link
Contributor Author

taimir commented Dec 11, 2015

Ok. thank you 👍

@taimir
Copy link
Contributor Author

taimir commented Dec 11, 2015

PTAL, should be ready for merge.

@@ -58,6 +58,7 @@ export default class NamespaceDialogController {
cancel() { this.mdDialog_.cancel(); }

/**
* @export
* Creates new namespace based on the state of the controller.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everywhere else @export and @private is the last line of comment so maybe switch this to be consistent.

@floreks
Copy link
Member

floreks commented Dec 11, 2015

Add @export also to isDisabled and cancel methods of NamespaceDialogController as they are used in template also.

@taimir taimir force-pushed the create_namespace_bugfix branch 2 times, most recently from d89b231 to 2ae6e59 Compare December 11, 2015 16:46
@taimir
Copy link
Contributor Author

taimir commented Dec 11, 2015

OK, just made it consistent. BTW, I notice the empty lines between the comment text and annotations are not really consistent throughout the code... Could we somehow unify this (agree on some rule)?

@floreks
Copy link
Member

floreks commented Dec 11, 2015

I agree, we should write this things down in some dev guide.

@bryk
Copy link
Contributor

bryk commented Dec 14, 2015

Thank you for this fix! We need to come up with a clever way to verify this during QA. In other projects I've worked on we required to send a live production demo of a change. But this is hard in open source... Let's talk about this tomorrow.

bryk added a commit that referenced this pull request Dec 14, 2015
Create namespace dialog bugfix (production)
@bryk bryk merged commit 1421182 into kubernetes:master Dec 14, 2015
@taimir taimir deleted the create_namespace_bugfix branch May 23, 2016 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants