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

Remove default parameter in updateInstructions() #1175

Merged
merged 1 commit into from
Sep 27, 2016
Merged

Remove default parameter in updateInstructions() #1175

merged 1 commit into from
Sep 27, 2016

Conversation

jerryaldrichiii
Copy link
Contributor

Some browsers do not support the use of default parameters.

Moving the logic that sets step to null if undefined into the function body allows those browsers to render the tutorial correctly.

This fixes #1165

@@ -68,7 +68,8 @@ export class AppComponent implements OnInit {
}

// called when tutorial commands need to be displayed
updateInstructions(step = null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

since we check the step value anyway, we do not have to set the default value, lets just use updateInstructions(step)

@@ -68,7 +68,8 @@ export class AppComponent implements OnInit {
}

// called when tutorial commands need to be displayed
updateInstructions(step = null) {
updateInstructions(step?) {
if (typeof step === 'undefined') { step = null; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we do not need this, since the switch statement should cover us

@jerryaldrichiii
Copy link
Contributor Author

jerryaldrichiii commented Sep 27, 2016

Agreed @chris-rock. PR has been updated 😄

Leaving updateInstructions(step?) instead of changing to updateInstructions(step) though because we call this method with no arguments at line 216.

@vjeffrey
Copy link

thanks @jerryaldrichiii !!! not sure what's up with the random travis test failing, and appveyor has an odd error too, since the change shouldn't have affected any of that. trying to look into it/see if i have permissions to rerun those

@jerryaldrichiii
Copy link
Contributor Author

Thanks @vjeffrey. Looks like a Ruby issue.

Specifically:

Gem::InstallError: term-ansicolor requires Ruby version >= 2.0

It looks like Ruby 1.9.3 is on those nodes:
https://ci.appveyor.com/project/chef/inspec/build/1.0.1918/job/dvp7ixv27wa0jho2#L312
https://travis-ci.org/chef/inspec/jobs/163080067#L354

@vjeffrey
Copy link

vjeffrey commented Sep 27, 2016

we discovered it was a problem with term-ansicolor, which requires ruby 2.0+ now, which is a dependency of inquirer. plan is to move that from the test group to a new group called exclude that group when bundle installing in travis

resolved with #1178

once this pr is rebased on master we should be g2g @jerryaldrichiii

Some browsers do not support the use of default parameters. Moving the
logic that sets `step` to null if undefined into the function body
allows those browsers to render the tutorial correctly.
@jerryaldrichiii
Copy link
Contributor Author

I rebased my branch to include your fix @vjeffrey.

Yay teamwork! ✋

@vjeffrey vjeffrey merged commit c66a2fa into inspec:master Sep 27, 2016
@jerryaldrichiii jerryaldrichiii deleted the remove-default-parameter-usage branch September 27, 2016 20:28
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.

Demo at http://inspec.io/ stuck on Loading
5 participants