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

fix(ivy): ensure component/directive class selectors are properly understood #27849

Closed
wants to merge 1 commit into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Dec 27, 2018

Angular allows for <ng-content> elements to include a selector which
filters which content-projected entries are inserted into the container
depending on whether or not the selector is matched.

With Ivy this feature has not fully worked due to the massive changes
that took place inside of Ivy's styling algorithm code (which is
responsible for assigning classes and styles to an element). This
fix ensures that content-projection can correctly identify which slot
an element should be placed into when class-based selectors are used.

@matsko matsko changed the title fix(ivy): ensure Component/Directive class selectors are properly understood fix(ivy): ensure component/directive class selectors are properly understood Dec 27, 2018
@matsko matsko force-pushed the issue_FW-833 branch 2 times, most recently from 9de6565 to 365c02e Compare December 27, 2018 19:21
@matsko matsko requested a review from mhevery December 27, 2018 19:21
@mary-poppins
Copy link

You can preview 9872d88 at https://pr27849-9872d88.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 9de6565 at https://pr27849-9de6565.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 365c02e at https://pr27849-365c02e.ngbuilds.io/.

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

Can you remove the fixmes for FW-833 (or replace desc with new root cause if there are additional issues)?

@kara kara added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews comp: ivy labels Dec 28, 2018
@ngbot ngbot bot added this to the needsTriage milestone Dec 28, 2018
@kara kara added the target: major This PR is targeted for the next major release label Dec 28, 2018
@mary-poppins
Copy link

You can preview 2277521 at https://pr27849-2277521.ngbuilds.io/.

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

@matsko Actually it looks like the tests added (for FW-833) are failing on CI. Can you take a look?

}
});
const main = TestBed.createComponent(MainComp);
fit('should project a single class-based tag', () => {

Choose a reason for hiding this comment

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

fit left

@mary-poppins
Copy link

You can preview 552b267 at https://pr27849-552b267.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 386a663 at https://pr27849-386a663.ngbuilds.io/.

@mary-poppins
Copy link

You can preview cf66990 at https://pr27849-cf66990.ngbuilds.io/.

@kara kara removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 11, 2019
@kara kara unassigned matsko Jan 11, 2019
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

LGTM

@matsko
Copy link
Contributor Author

matsko commented Jan 11, 2019

@mary-poppins
Copy link

You can preview 34979bd at https://pr27849-34979bd.ngbuilds.io/.

@matsko
Copy link
Contributor Author

matsko commented Jan 14, 2019

…nderstood

Angular allows for `<ng-content>` elements to include a selector which
filters which content-projected entries are inserted into the container
depending on whether or not the selector is matched.

With Ivy this feature has not fully worked due to the massive changes
that took place inside of Ivy's styling algorithm code (which is
responsible for assigning classes and styles to an element). This
fix ensures that content-projection can correctly identify which slot
an element should be placed into when class-based selectors are used.
@mary-poppins
Copy link

You can preview cc14acc at https://pr27849-cc14acc.ngbuilds.io/.

@matsko matsko added the action: merge The PR is ready for merge by the caretaker label Jan 15, 2019
pkozlowski-opensource pushed a commit to pkozlowski-opensource/angular that referenced this pull request Jan 15, 2019
…nderstood

Angular allows for `<ng-content>` elements to include a selector which
filters which content-projected entries are inserted into the container
depending on whether or not the selector is matched.

With Ivy this feature has not fully worked due to the massive changes
that took place inside of Ivy's styling algorithm code (which is
responsible for assigning classes and styles to an element). This
fix ensures that content-projection can correctly identify which slot
an element should be placed into when class-based selectors are used.

Closes angular#27849
@matsko matsko deleted the issue_FW-833 branch January 15, 2019 23:14
wKoza pushed a commit to wKoza/angular that referenced this pull request Jan 18, 2019
…nderstood (angular#27849)

Angular allows for `<ng-content>` elements to include a selector which
filters which content-projected entries are inserted into the container
depending on whether or not the selector is matched.

With Ivy this feature has not fully worked due to the massive changes
that took place inside of Ivy's styling algorithm code (which is
responsible for assigning classes and styles to an element). This
fix ensures that content-projection can correctly identify which slot
an element should be placed into when class-based selectors are used.

PR Close angular#27849
wKoza pushed a commit to wKoza/angular that referenced this pull request Jan 18, 2019
…nderstood (angular#27849)

Angular allows for `<ng-content>` elements to include a selector which
filters which content-projected entries are inserted into the container
depending on whether or not the selector is matched.

With Ivy this feature has not fully worked due to the massive changes
that took place inside of Ivy's styling algorithm code (which is
responsible for assigning classes and styles to an element). This
fix ensures that content-projection can correctly identify which slot
an element should be placed into when class-based selectors are used.

PR Close angular#27849
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
…nderstood (angular#27849)

Angular allows for `<ng-content>` elements to include a selector which
filters which content-projected entries are inserted into the container
depending on whether or not the selector is matched.

With Ivy this feature has not fully worked due to the massive changes
that took place inside of Ivy's styling algorithm code (which is
responsible for assigning classes and styles to an element). This
fix ensures that content-projection can correctly identify which slot
an element should be placed into when class-based selectors are used.

PR Close angular#27849
@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

6 participants