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

Dhaulagiri · 2015-09-14T15:05:44Z

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 · 2015-09-14T15:15:46Z

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

rwjblue · 2015-09-14T15:23:03Z

Yes, at least these:

rwjblue · 2015-09-14T15:23:55Z

But the class ones need to be fixed in master also: https://github.com/emberjs/ember.js/blob/master/packages/ember-routing-views/lib/components/link-to.js#L748-L762

Dhaulagiri · 2015-09-14T15:37:41Z

@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 · 2015-09-14T15:49:26Z

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 · 2015-09-14T16:17:07Z

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

stefanpenner · 2015-09-14T16:20:57Z

packages/ember-routing-views/lib/views/link.js

    }

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

    if (attrs.currentWhen) {


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

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

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

stefanpenner · 2015-09-14T16:21:35Z

i think https://github.com/emberjs/ember.js/pull/12344/files#diff-f3c536e535ba1d44c39353a4bc2d6496R432 also needs this.

Dhaulagiri · 2015-09-14T16:25:01Z

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

rwjblue · 2015-09-14T16:25:09Z

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 · 2015-09-14T16:37:44Z

Removed that block and squashed all the commits

rwjblue · 2015-09-14T17:20:56Z

👍 - 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 · 2015-09-14T17:26:32Z

Sure thing, I'll do that this afternoon.

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

rwjblue · 2015-09-14T18:42:49Z

Awesome, thank you!

stefanpenner · 2015-09-14T18:47:32Z

👍

Dhaulagiri · 2015-09-14T18:54:28Z

Thanks @rwjblue @stefanpenner for the assistance!

Dhaulagiri · 2015-09-25T13:13:27Z

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

Dhaulagiri · 2015-09-25T13:27:05Z

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.

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

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)

# 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`

stefanpenner reviewed Sep 14, 2015
View reviewed changes

[BUGFIX release-1-13] Use getValue in attrs lookups

7f45759

* 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 mentioned this pull request Sep 14, 2015

[BUGFIX beta] Clean up link-to classes #12345

Merged

rwjblue added a commit that referenced this pull request Sep 14, 2015

Merge pull request #12344 from Dhaulagiri/br-fix-current-when

a527955

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

rwjblue merged commit a527955 into emberjs:release-1-13 Sep 14, 2015

Dhaulagiri deleted the br-fix-current-when branch September 14, 2015 18:54

Dhaulagiri mentioned this pull request Sep 14, 2015

currentWhen.split is not a function #12296

Closed

jcope2013 mentioned this pull request Sep 16, 2015

{{link-to}} "disabledWhen" documentation wrong? tildeio/htmlbars#413

Closed

rwjblue mentioned this pull request Apr 10, 2016

[BUGFIX release] allow current-when to be a boolean #12512 #12630

Closed

locks mentioned this pull request Apr 26, 2016

link-to current-when cannot be supplied as true as per docs #12512

Closed

whatthewhat mentioned this pull request Jan 18, 2017

[BUGFIX beta] Allow boolean values for current-when #14848

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Dhaulagiri commented Sep 14, 2015

stefanpenner commented Sep 14, 2015

rwjblue commented Sep 14, 2015

rwjblue commented Sep 14, 2015

Dhaulagiri commented Sep 14, 2015

rwjblue commented Sep 14, 2015

Dhaulagiri commented Sep 14, 2015

stefanpenner Sep 14, 2015

Dhaulagiri Sep 14, 2015

rwjblue Sep 14, 2015

stefanpenner commented Sep 14, 2015

Dhaulagiri commented Sep 14, 2015

rwjblue commented Sep 14, 2015

Dhaulagiri commented Sep 14, 2015

rwjblue commented Sep 14, 2015

Dhaulagiri commented Sep 14, 2015

rwjblue commented Sep 14, 2015

stefanpenner commented Sep 14, 2015

Dhaulagiri commented Sep 14, 2015

Dhaulagiri commented Sep 25, 2015

Dhaulagiri commented Sep 25, 2015

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

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

Conversation

Dhaulagiri commented Sep 14, 2015

stefanpenner commented Sep 14, 2015

rwjblue commented Sep 14, 2015

rwjblue commented Sep 14, 2015

Dhaulagiri commented Sep 14, 2015

rwjblue commented Sep 14, 2015

Dhaulagiri commented Sep 14, 2015

stefanpenner Sep 14, 2015

Choose a reason for hiding this comment

Dhaulagiri Sep 14, 2015

Choose a reason for hiding this comment

rwjblue Sep 14, 2015

Choose a reason for hiding this comment

stefanpenner commented Sep 14, 2015

Dhaulagiri commented Sep 14, 2015

rwjblue commented Sep 14, 2015

Dhaulagiri commented Sep 14, 2015

rwjblue commented Sep 14, 2015

Dhaulagiri commented Sep 14, 2015

rwjblue commented Sep 14, 2015

stefanpenner commented Sep 14, 2015

Dhaulagiri commented Sep 14, 2015

Dhaulagiri commented Sep 25, 2015

Dhaulagiri commented Sep 25, 2015