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

feat(compile): add human readable alternative to '@=&?' #9137

Closed
wants to merge 1 commit into from

Conversation

@shahata
Copy link
Contributor

shahata commented Sep 17, 2014

support using interpolate:<name>, bind:<name>, eval:<name> and bind:optional:<name> instead of @<name>, =<name>, &<name> and =?<name>

Closes #9125

@shahata shahata force-pushed the shahata:verbose-scope branch from 7045fe7 to 0491692 Sep 17, 2014
@mary-poppins mary-poppins added cla: yes and removed cla: no labels Sep 17, 2014
@shahata shahata force-pushed the shahata:verbose-scope branch from 0491692 to 45dc0f9 Sep 17, 2014
@caitp
Copy link
Contributor

caitp commented Sep 17, 2014

it looks alright to me, I think we do need another core team opinion on this though --- @tbosch wdyt?

@caitp caitp added this to the 1.3.0 milestone Sep 17, 2014
expect(componentScope.optRefAlias).toBe(componentScope.optRef);
expect(componentScope.optRefAlias).toBe(undefined);
expect(componentScope.optRefVerbose).toBe(undefined);
expect(componentScope.optRefAliasVerbose).toBe(undefined);

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 17, 2014

Member

The above 4 checks are incorrect (not just the new "verbose" ones, but the old ones as well).
We are checking against componentScope.optRef[XYZ], but they will always be undefined, because there are no such properties on componentScope.

We should be checking against componentScope.optref[XYZ] (notice the lowercase r), because these are the properties that will eventually get values, so these are the ones we want to ensure are undefined before $rootScope.name gets a value assigned.

(The rest of the it block uses optref[XYZ] correctly.

This comment has been minimized.

Copy link
@shahata

shahata Sep 17, 2014

Author Contributor

good catch. thanks.

support using `interpolate:<name>`, `bind:<name>`, `eval:<name>` and `bind:optional:<name>` instead of `@<name>`, `=<name>`, `&<name>` and `=?<name>`

Closes #9125
@shahata shahata force-pushed the shahata:verbose-scope branch from 45dc0f9 to 808380d Sep 17, 2014
@rcollette

This comment has been minimized.

Copy link

rcollette commented on 808380d Sep 17, 2014

How about ^ for the parent controller?

This comment has been minimized.

Copy link
Contributor Author

shahata replied Sep 17, 2014

let's first agree on the general idea, then we can think where else we can use this methodology

@btford
Copy link
Contributor

btford commented Sep 22, 2014

@tbosch thoughts? I like the syntax better, but idk if it's really worth changing Angular 1.x to have yet another way to do this...

@caitp
Copy link
Contributor

caitp commented Sep 22, 2014

agree that having multiple ways to do it kind of sucks, but if we're going to do it, then this doesn't look like a bad way

@jeffbcross jeffbcross force-pushed the angular:master branch from dbbabb9 to 27d1234 Sep 27, 2014
@jeffbcross jeffbcross modified the milestones: 1.3.0, Backlog Sep 29, 2014
@jeffbcross jeffbcross force-pushed the angular:master branch from cad9560 to f294244 Oct 2, 2014
@tbosch
Copy link
Contributor

tbosch commented Oct 3, 2014

Just talked to @IgorMinar, for now we don't do this as it adds another way to do the same thing. Would need to keep both in sync, update the style guide, ...

But we should document why we use those symbols

  • @: access attributes just like in css selectors
  • &: from C to keep a reference to a function
  • =: obvious
@tbosch
Copy link
Contributor

tbosch commented Oct 3, 2014

@shahata Could you create a docs PR instead with these explanations?

@btford btford force-pushed the angular:master branch from 9011a65 to 46db47b Oct 6, 2014
@jeffbcross jeffbcross force-pushed the angular:master branch 4 times, most recently from e8dc429 to e83fab9 Oct 8, 2014
@petebacondarwin petebacondarwin force-pushed the angular:master branch from 4dd5a20 to 998c61c Oct 19, 2014
@caitp
Copy link
Contributor

caitp commented Dec 10, 2014

@shahata ping --- did you ever submit docs PRs about this? Someone on the IRC channel was saying we really need better docs/more examples for the isolate scope variable symbols.

@shahata
Copy link
Contributor Author

shahata commented Dec 13, 2014

Sorry, somehow missed this one. I have to say that the reasoning behind the symbols doesn't make it any easier to understand imo. Even for me it sounds a bit strange:

  1. Where do we use @ in css selectors? The only use of @ in css that I'm aware of is for instructions. Maybe @ is taken from xpath but still I don't think it will help a lot of people to understand.
  2. I'm an old C hacker, so I'm pretty sure & is for getting the address of a variable (and in C++ for passing function arguments by reference), so I don't get the analogy and I don't think many people will...

I think we'd better close this unless someone has a good suggestion regarding how the docs can be improved.

@googlebot
Copy link

googlebot commented Dec 13, 2014

CLAs look good, thanks!

@lgalfaso
Copy link
Member

lgalfaso commented Jan 2, 2015

Based on #9137 (comment), this is not going to land

@rcollette
Copy link

rcollette commented Jan 2, 2015

What a shame this was dismissed. When you read blogs, instructional videos,etc this is commonly discussed as a pain point. Keeping documentation up to date is a lame excuse for not implementing a small but helpful bit of code. I sense that fragile egos related to the original design are more the issue. Not one reason was presented for using symbology here and not elsewhere in angular, yet were going to hold on to it.

@lgalfaso
Copy link
Member

lgalfaso commented Jan 2, 2015

@rcollette please keep the conversation technical and follow the code of conduct

@rcollette
Copy link

rcollette commented Jan 2, 2015

My apologies

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

10 participants
You can’t perform that action at this time.