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

refactor(ivy): ensure directive host bindings use the styling algorithm #27134

Closed
wants to merge 3 commits into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Nov 16, 2018

No description provided.

@mary-poppins
Copy link

You can preview b5bb890 at https://pr27134-b5bb890.ngbuilds.io/.

@mary-poppins
Copy link

You can preview b1b227e at https://pr27134-b1b227e.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d98994c at https://pr27134-d98994c.ngbuilds.io/.

@@ -1111,7 +1111,7 @@ export function elementClassProp(
export function elementStyling(
classDeclarations?: (string | boolean | InitialStylingFlags)[] | null,
styleDeclarations?: (string | boolean | InitialStylingFlags)[] | null,
styleSanitizer?: StyleSanitizeFn | null): void {
styleSanitizer?: StyleSanitizeFn | null, directiveIndex?: number): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

need docs for the extra argument.

@@ -1153,7 +1153,7 @@ export function elementStyling(
* specifically for element styling--the index must be the next index after the element
* index.)
*/
export function elementStylingApply(index: number): void {
export function elementStylingApply(index: number, directiveIndex?: number): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

need docs for the extra argument.

@@ -50,15 +51,21 @@ export interface StylingInstruction {
*
* The creation/update methods within the builder class produce these instructions.
*/
interface BindingEntry {
Copy link
Contributor

Choose a reason for hiding this comment

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

You inserted the interface in between the comment and class which means that the interface now has the comment of StyleBuilder.

Can we come up with a better name for BindingEntry Perhaps StyleBoundAttribute or something like that?

const _c0 = ["foo", "baz", ${InitialStylingFlags.VALUES_MODE}, "foo", true, "baz", true];
const _c1 = ["width", "height", "color", ${InitialStylingFlags.VALUES_MODE}, "width", "200px", "height", "500px"];
hostBindings: function MyComponent_HostBindings(dirIndex, elIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kara I think we may need to do the refactoring for host bindings sooner than later. The correct output here should be

          hostBindings: function MyComponent_HostBindings(rf:RenderFlags, dirIndex, elIndex) {
            if (rf & RenderFlags.Create) {
              $r3$.ɵelementStyling(_c0, _c1, $r3$.ɵdefaultStyleSanitizer, dirIndex);
            }
            if (rf & RenderFlags.Update) {
              $r3$.ɵelementStylingMap(elIndex, $r3$.ɵload(dirIndex).myClass, $r3$.ɵload(dirIndex).myStyle, dirIndex);
              $r3$.ɵelementStyleProp(elIndex, 2, $r3$.ɵload(dirIndex).myColorProp, null, dirIndex);
              $r3$.ɵelementClassProp(elIndex, 0, $r3$.ɵload(dirIndex).myFooClass, dirIndex);
              $r3$.ɵelementStylingApply(elIndex, dirIndex);
            }
          }

The key here is that the call to elementStyling should happen only once at initialization, where as the rest of the call need to happen one each binding.

@matsko How are you planning to deal with the above until @kara fixes the runtime?
@matsko Is it in issue that we will have multiple calls to elementStylingApply From view as well as from host bindings?

@mhevery mhevery added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Nov 16, 2018
@mary-poppins
Copy link

You can preview 2a34661 at https://pr27134-2a34661.ngbuilds.io/.

@mhevery mhevery closed this in b5dbf51 Nov 17, 2018
mhevery added a commit that referenced this pull request Nov 17, 2018
@mhevery
Copy link
Contributor

mhevery commented Nov 20, 2018

Merged as #27145

FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants