Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngHref): allow setting href to empty string #6983

Closed
wants to merge 1 commit into from

Conversation

nickeddy
Copy link

@nickeddy nickeddy commented Apr 3, 2014

Fixed issue 2755:
#2755

ngHref should now allow empty strings, and ngSrc will not accept empty strings as intended: b6e4a71

@caitp
Copy link
Contributor

caitp commented Apr 3, 2014

You need a test, it should be easy to write:

  it('should remove href value when interpolated value is empty', inject(function($compile, $rootScope) {
    element = $compile('<div ng-href="{{id}}"></div>')($rootScope)
    $rootScope.$digest();
    expect(element.attr('href')).toEqual('');

    $rootScope.$apply(function() {
      $rootScope.id = 1;
    });
    expect(element.attr('href')).toEqual('1');
  }));

Or something (I don't know if that test will pass yet, but the interpolated value won't be empty if you put ng-href="someString{{interpolated}}", because the interpolated value is always the concatenation of the raw string stuff and the expression's toString()'d value

	modified:   src/ng/directive/booleanAttrs.js
	modified:   test/ng/directive/booleanAttrsSpec.js
@shahata
Copy link
Contributor

shahata commented Apr 3, 2014

This is is breaking the current behavior of not setting the href attribute when the value is empty. It will result in href="" which will point to the current base href. Also the test doesn't make sense since it tests transition from empty to not empty and not vice versa. Also 2, I thought it was clear that I started working on this PR. Uncool. :(

@caitp
Copy link
Contributor

caitp commented Apr 4, 2014

hmm, @shahata has a point. Maybe removing the attribute entirely would make more sense

@nickeddy nickeddy added cla: yes and removed cla: no labels Apr 4, 2014
@chrisirhc
Copy link
Contributor

+1 Just ran into this. Unable to disable a link without another directive that does preventDefault.

@shahata
Copy link
Contributor

shahata commented Jun 17, 2014

This is solved more thoroughly by #6986

@caitp
Copy link
Contributor

caitp commented Jun 17, 2014

@shahata your CL is bitrotten because of matsko's change to attrs, needs a rebase. Also, I suggest not calling the scope "booleanAttrs", it's specific to ngHref, imo

@btford btford added this to the Backlog milestone Jul 23, 2014
@btford
Copy link
Contributor

btford commented Jul 23, 2014

Superseded by 469ea33. Thanks though, @nickeddy!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants