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

[BUGFIX release-1-13] Allow current-when to be specified via a variable #12344

Merged
merged 1 commit into from
Sep 14, 2015
Merged

[BUGFIX release-1-13] Allow current-when to be specified via a variable #12344

merged 1 commit into from
Sep 14, 2015

Conversation

Dhaulagiri
Copy link
Contributor

Per guidance from @stefanpenner this fixes the issue where we are not able to set current-when to a variable property in 1.13.

Fixes #12296

@stefanpenner
Copy link
Member

Awesome. I suspect each of the other attr gets in that function should also use getValue

@rwjblue
Copy link
Member

rwjblue commented Sep 14, 2015

@Dhaulagiri
Copy link
Contributor Author

@stefanpenner @rwjblue I'm happy to update this PR to fix the others in that file if you'd like. Something like this:

if (attrs.disabledClass) {
    this.set('disabledClass', attrs.disabledClass);
  }

becomes

var disabledClass = getValue(attrs.disabledClass);
if (disabledClass) {
    this.set('disabledClass', disabledClass);
  }

?

Unsure how/if to test these other cases.

@rwjblue
Copy link
Member

rwjblue commented Sep 14, 2015

Thanks for tackling!

The fix for activeClass and disabledClass is to remove the massaging done there completely, by default all attributes provided in attrs.* are available on the root of the component with the same name (so no work is needed since we are using the same key in both).

For disabledWhen I think we would only want to getValue if attrs.disabledWhen exists:

    if (attrs.disabledWhen) {
      this.set('disabled', getValue(attrs.disabledWhen));
    }

Roughly the same test will work, basically use a bound param for activeClass/disabledClass and confirm that it is on the element when active/disabled.

@Dhaulagiri
Copy link
Contributor Author

@rwjblue have a look when you get a chance and see if I was able to follow your direction correctly

}

var currentWhen = attrs['current-when'];
var currentWhen = getValue(attrs['current-when']);

if (attrs.currentWhen) {
Copy link
Member

Choose a reason for hiding this comment

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

416 and 418 feel somewhat misaligned. we should use currentWhen in both cases, or refactor to improve clarity of intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the same thing but didn't act because of the deprecation and this being 1.13 and all

Copy link
Member

Choose a reason for hiding this comment

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

Yes, agreed. Lets do go forward refactorings on master (so they get more battle tested through our canary -> beta -> stable process).

@stefanpenner
Copy link
Member

@Dhaulagiri
Copy link
Contributor Author

@stefanpenner I think that block could just be removed like with activeClass/disabledClass

@rwjblue
Copy link
Member

rwjblue commented Sep 14, 2015

Yep, looks like what I had in mind. I think this is good once you remove the section @stefanpenner just mentioned.

* Fixes a bug with allowing current-when to be specified via a bound param
* Removes unneccessary massaging of activeClass/disableClass/loadingClass attrs
* Use getValue to lookup disabledWhen
* Add tests
@Dhaulagiri
Copy link
Contributor Author

Removed that block and squashed all the commits

@rwjblue
Copy link
Member

rwjblue commented Sep 14, 2015

👍 - Looks good to me!

@Dhaulagiri - Would you mind making a PR to master adding these tests and removing the extra unneeded blocks (labeled [BUGFIX beta])?

@Dhaulagiri
Copy link
Contributor Author

Sure thing, I'll do that this afternoon.

rwjblue added a commit that referenced this pull request Sep 14, 2015
[BUGFIX release-1-13] Allow current-when to be specified via a variable
@rwjblue rwjblue merged commit a527955 into emberjs:release-1-13 Sep 14, 2015
@rwjblue
Copy link
Member

rwjblue commented Sep 14, 2015

Awesome, thank you!

@stefanpenner
Copy link
Member

👍

@Dhaulagiri
Copy link
Contributor Author

Thanks @rwjblue @stefanpenner for the assistance!

@Dhaulagiri
Copy link
Contributor Author

@rwjblue do you know when you might cut a new release of 1.13 with this in it?

@Dhaulagiri
Copy link
Contributor Author

Actually, I just saw in a linked issue how I can point to the release-1-13 branch, so this is not particularly urgent for us.

whatthewhat added a commit to whatthewhat/ember.js that referenced this pull request Jan 18, 2017
As the docs say, `A link will be active if current-when is true`.
Looks like this might have been broken since 1.13 and emberjs#12344
did not seem to actually fix this particular bug.

Related issues:

- emberjs#12512
- emberjs#12630 (fix was not merged)
- emberjs#12296
whatthewhat added a commit to whatthewhat/ember.js that referenced this pull request Jun 26, 2017
As the docs say, `A link will be active if current-when is true`.
Looks like this might have been broken since 1.13 and emberjs#12344
did not seem to actually fix this particular bug.

Related issues:

- emberjs#12512
- emberjs#12630 (fix was not merged)
- emberjs#12296
rwjblue pushed a commit that referenced this pull request Aug 8, 2017
As the docs say, `A link will be active if current-when is true`.
Looks like this might have been broken since 1.13 and #12344
did not seem to actually fix this particular bug.

Related issues:

- #12512
- #12630 (fix was not merged)
- #12296

(cherry picked from commit d0f320a)
locks added a commit that referenced this pull request Aug 16, 2017
# This is the 1st commit message:
typo in comment

# The commit message #2 will be skipped:

#	indent yuidoc comment

# The commit message #3 will be skipped:

#	remove IE8 test

# The commit message #4 will be skipped:

#	remove commented out test

# The commit message #5 will be skipped:

#	use file path and add imports
#
#	Used RFC #176 modules API for imports.
#	Also cleaned up some of the globals-mode usage.

# The commit message #6 will be skipped:

#	clean up more globals style documentation

# The commit message #7 will be skipped:

#	Updates blueprints

# The commit message #8 will be skipped:

#	remove extra type check

# The commit message #9 will be skipped:

#	[BUGFIX beta] Reusing element causes problems in Safari
#
#	When testing allowed input types, in some versions of Safari the type
#	cannot be change to `file` if previously set to a different one.
#
#	Fixes #14727

# The commit message #10 will be skipped:

#	[DOC release]missed code block added

# The commit message #1 will be skipped:

#	[DOC release] Update wait.js - Add missing backticks to code snippet.

# The commit message #2 will be skipped:

#	use safe `toString` for array content in `mixins/array`

# The commit message #3 will be skipped:

#	avoid expanding already expanded property key in computed.sort

# The commit message #4 will be skipped:

#	avoid expanding already expanded property key in reduceMacro/arrayMacro/multiArrayMacro

# The commit message #5 will be skipped:

#	[DOC release] Make `Ember.expandProperties` public

# The commit message #6 will be skipped:

#	reuse meta `arrayContentDidChange`

# The commit message #7 will be skipped:

#	replace `throw` with assertion in `enumerable`

# The commit message #8 will be skipped:

#	[BUGFIX beta] Allow boolean values for current-when
#
#	As the docs say, `A link will be active if current-when is true`.
#	Looks like this might have been broken since 1.13 and #12344
#	did not seem to actually fix this particular bug.
#
#	Related issues:
#
#	- #12512
#	- #12630 (fix was not merged)
#	- #12296

# The commit message #9 will be skipped:

#	remove unused imports

# The commit message #10 will be skipped:

#	[DOC] Improve Ember.isEmpty

# The commit message #1 will be skipped:

#	micro optimization in `enumerable`
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.

3 participants