docs(core): improve viewEncapsulation documentation by dario-piotrowicz · Pull Request #44151 · angular/angular

dario-piotrowicz · 2021-11-11T20:27:07Z

Slighlty improve the viewEncapsulation documentation (both in code
comments and content files) to make it more clear and understandable.

See #44099 (comment)

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Other information

@AndrewKushnir as promised 🙂

AndrewKushnir · 2021-11-11T20:39:06Z

@dario-piotrowicz thanks for creating this PR 👍
I'd like to ask if @petebacondarwin can take a look, since he worked with this subsystem relatively recently.

dario-piotrowicz · 2021-11-11T20:32:17Z

I've removed the example as it didn't seem beneficial to me as:

after my changes the introductory text above already makes clear how to apply the strategy so I don't really think actually showing it adds much here

in the same docs page (below) there are various examples showing the application of the encapsulation strategy, so this example again didn't seem to add much to me

dario-piotrowicz · 2021-11-11T20:39:00Z

this was already mentioned above so I don't think there is a need to emphasise it

dario-piotrowicz · 2021-11-11T20:43:28Z

I changed "The support is still limited" to "Not all browsers support it" because as you can see in the caniuse link the support is actually pretty good I think

dario-piotrowicz · 2021-11-11T20:48:00Z

I've removed this because as mentioned in this comment I feel like it is an unnecessary example

dario-piotrowicz · 2021-11-11T20:52:31Z

maybe it should be is-helpful? 🤷‍♂️

Don't all browsers that we officially support have ShadowDOM support now?

I think the real reason we are still defaulting to emulated is that is makes it hard to pierce theming into the components because there is not a well lit path for how to do this with ShadowDOM. I think @jelbourn has more context here.

yes I wrote something similar in my comment below, happy to change this to whatever is preferred 🙂

I think this is fine unless @jelbourn (or anyone else has a view).

petebacondarwin · 2021-11-12T12:09:13Z

@dario-piotrowicz - while I review this PR can you fix up the commit message slightly? Add some sentence casing and fix a typo:

docs(core): improve viewEncapsulation documentation 

Slighlty improve the `viewEncapsulation` documentation (both in code
comments and content files) to make it more clear and understandable.

See https://github.com/angular/angular/pull/44099#discussion_r745890903

mary-poppins · 2021-11-12T12:09:34Z

You can preview c902a56 at https://pr44151-c902a56.ngbuilds.io/.
You can preview 00e3081 at https://pr44151-00e3081.ngbuilds.io/.
You can preview 73bde89 at https://pr44151-73bde89.ngbuilds.io/.
You can preview 496cbbe at https://pr44151-496cbbe.ngbuilds.io/.
You can preview fdc735e at https://pr44151-fdc735e.ngbuilds.io/.

Slighlty improve the `viewEncapsulation` documentation (both in code comments and content files) to make it more clear and understandable. See angular#44099 (comment)

mary-poppins · 2021-11-12T13:20:58Z

You can preview f5c611c at https://pr44151-f5c611c.ngbuilds.io/.

petebacondarwin

Thanks for working on this @dario-piotrowicz - definitely an improvement. I made a bunch of suggestions - mostly just stylistic.

petebacondarwin · 2021-11-12T12:13:12Z

I think the docs team prefer to keep an imperative feel to the docs. So probably it would be best to revert this change:

Suggested change

The available options are:

Choose from the following modes:

ok 🙂 👍

but my issue here was especially that before mode made more sense as we had ShadowDom, Emulated and None, to which "mode" was more appropriate, but now we have ViewEncapsulation.ShadowDom, ViewEncapsulation.Emulated and ViewEncapsulation.None, which are actual specific value of the ViewEncapsulation enum, so I just wanted to rephrase it to indicate such.

What do you think? should I revert also the names to the old version?

dario-piotrowicz · 2021-11-12T21:26:33Z

@petebacondarwin thanks a lot for the awesome reviewing 🙂 , sorry I made you comment that much 😓

I replied to most of you comments, I've asked follow up questions to a few, please have another look when you have the chance 🙂

mary-poppins · 2021-11-12T21:30:26Z

You can preview 8308abe at https://pr44151-8308abe.ngbuilds.io/.

mary-poppins · 2021-11-12T22:00:14Z

You can preview 7599409 at https://pr44151-7599409.ngbuilds.io/.

petebacondarwin

Thanks for the updates @dario-piotrowicz - LGTM!!

petebacondarwin · 2021-11-13T11:58:52Z

I think this is fine unless @jelbourn (or anyone else has a view).

mary-poppins · 2021-11-13T22:56:49Z

You can preview 94a6c10 at https://pr44151-94a6c10.ngbuilds.io/.

atscott

LGTM though my knowledge in this area is limited.

ngbot · 2021-11-24T18:37:58Z

I see that you just added the action: merge label, but the following checks are still failing:
status "ci/circleci: legacy-unit-tests-saucelabs" is failing
1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

AndrewKushnir · 2021-11-24T18:39:14Z

FYI, marking this PR as ready for merge, since we have all the necessary approvals.

Note to the Caretaker: the CI job failure is unrelated, so we can proceed with the merge.

jessicajaniuk · 2021-11-24T18:53:47Z

This PR was merged into the repository by commit 6ae3858.

Slighlty improve the `viewEncapsulation` documentation (both in code comments and content files) to make it more clear and understandable. See #44099 (comment) PR Close #44151

angular-automatic-lock-bot · 2021-12-25T16:06:11Z

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.}

google-cla Bot added the cla: yes label Nov 11, 2021

pullapprove Bot requested a review from atscott November 11, 2021 20:27

AndrewKushnir requested a review from petebacondarwin November 11, 2021 20:38

AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer comp: docs target: patch This PR is targeted for the next patch release labels Nov 11, 2021

ngbot Bot added this to the Backlog milestone Nov 11, 2021

dario-piotrowicz commented Nov 11, 2021

View reviewed changes

dario-piotrowicz force-pushed the viewEncapsulation-docs branch from c902a56 to 00e3081 Compare November 11, 2021 20:46

dario-piotrowicz commented Nov 11, 2021

View reviewed changes

dario-piotrowicz force-pushed the viewEncapsulation-docs branch 3 times, most recently from 496cbbe to fdc735e Compare November 11, 2021 21:32

petebacondarwin changed the title ~~docs(core): improve viewEncalsulation documentation~~ docs(core): improve viewEncapsulation documentation Nov 12, 2021

petebacondarwin added the aio: preview label Nov 12, 2021

docs(core): improve viewEncapsulation documentation

f5c611c

Slighlty improve the `viewEncapsulation` documentation (both in code comments and content files) to make it more clear and understandable. See angular#44099 (comment)

dario-piotrowicz force-pushed the viewEncapsulation-docs branch from fdc735e to f5c611c Compare November 12, 2021 13:12

petebacondarwin suggested changes Nov 12, 2021

View reviewed changes

dario-piotrowicz requested a review from petebacondarwin November 12, 2021 21:26

fixup! docs(core): improve viewEncapsulation documentation

7599409

dario-piotrowicz force-pushed the viewEncapsulation-docs branch from 8308abe to 7599409 Compare November 12, 2021 21:51

petebacondarwin approved these changes Nov 13, 2021

View reviewed changes

petebacondarwin requested a review from jelbourn November 13, 2021 11:59

fixup! docs(core): improve viewEncapsulation documentation

94a6c10

atscott approved these changes Nov 18, 2021

View reviewed changes

AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 24, 2021

AndrewKushnir added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Nov 24, 2021

jessicajaniuk closed this in 6ae3858 Nov 24, 2021

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Dec 25, 2021

Conversation

dario-piotrowicz commented Nov 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

AndrewKushnir commented Nov 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin commented Nov 12, 2021

Uh oh!

mary-poppins commented Nov 12, 2021

Uh oh!

mary-poppins commented Nov 12, 2021

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dario-piotrowicz commented Nov 12, 2021

Uh oh!

mary-poppins commented Nov 12, 2021

Uh oh!

mary-poppins commented Nov 12, 2021

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Nov 13, 2021

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

ngbot Bot commented Nov 24, 2021

Uh oh!

AndrewKushnir commented Nov 24, 2021

Uh oh!

jessicajaniuk commented Nov 24, 2021

Uh oh!

angular-automatic-lock-bot Bot commented Dec 25, 2021

Uh oh!

Reviewers

Assignees

Labels

dario-piotrowicz commented Nov 11, 2021 •

edited

Loading