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

[cssom-1, css-cascade-5] Add adoptedStyleSheets using ObservableArray #6431

Merged
merged 4 commits into from Jul 20, 2021
Merged

[cssom-1, css-cascade-5] Add adoptedStyleSheets using ObservableArray #6431

merged 4 commits into from Jul 20, 2021

Conversation

mfreed7
Copy link

@mfreed7 mfreed7 commented Jul 7, 2021

[cssom-1, css-cascade-5] Add adoptedStyleSheets using ObservableArray.

Some things I need input on:

  1. See this discussion. I added a single list item to the Cascade-5 text to reference the new "final CSS style sheets" concept I added in CSSOM-1. We might (?) want to instead change the styleSheets property to explicitly exclude the contents of adoptedStyleSheets, and fold "final CSS style sheets" back into "document or shadow root CSS style sheets".
  2. I can't seem to get the "link magic" working between cssom-1 and css-cascade-5. I tried various permutations of <a for=foobar>concept name</a> but nothing seemed to please bikeshed. So I resorted to <a href=>. Help appreciated.

Credit to @domenic for a draft of this PR.

@emilio

Closes WICG/construct-stylesheets#118, Closes WICG/construct-stylesheets#93

@dbaron
Copy link
Member

dbaron commented Jul 8, 2021

I can't seem to get the "link magic" working between cssom-1 and css-cascade-5. I tried various permutations of concept name but nothing seemed to please bikeshed. So I resorted to <a href=>. Help appreciated.

I believe bikeshed's only mode of operation is processing one spec at a time, relative to the published (editor's draft or TR) versions of other specs. I don't think there's a way to process multiple drafts at once to test whether cross-draft linking changes that require edits to multiple drafts will work. I think the thing to do is look at what generally works elsewhere, do that, and check it afterwards.

Copy link
Collaborator

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Yeah, the contents look good to me, but I'm pretty sure the linking needs work. Maybe it's worth landing this in different PRs assuming there's no circular dependency?

cssom-1/Overview.bs Outdated Show resolved Hide resolved
@mfreed7
Copy link
Author

mfreed7 commented Jul 15, 2021

Yeah, the contents look good to me, but I'm pretty sure the linking needs work. Maybe it's worth landing this in different PRs assuming there's no circular dependency?

Thanks for the comments here. I'm happy to land the change to css-cascade-5 in a later PR, but see my comment above. I can't seem to get even the internal references to work. I think something about the <code> block or something, but nothing I tried worked. I'm sure it's easy and I'm just missing it.

cssom-1/Overview.bs Outdated Show resolved Hide resolved
cssom-1/Overview.bs Outdated Show resolved Hide resolved
@@ -1035,6 +1035,7 @@ Cascade Sorting Order</h3>
For this purpose:

<ul>
<li>Style sheets are ordered as in <a href="https://www.w3.org/TR/cssom-1/#documentorshadowroot-final-css-style-sheets">final CSS style sheets</a>.
Copy link
Author

Choose a reason for hiding this comment

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

I left the HREF here for now, and I'll land a followup CL converting it to a bikeshed link after this lands, if that's ok with you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<li>Style sheets are ordered as in <a href="https://www.w3.org/TR/cssom-1/#documentorshadowroot-final-css-style-sheets">final CSS style sheets</a>.
<li>Style sheets are ordered as in <a href="https://drafts.csswg.org/cssom/#documentorshadowroot-final-css-style-sheets">final CSS style sheets</a>.

since the TR URL there is stuck in 2016.

Copy link
Author

Choose a reason for hiding this comment

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

Oops, better, thanks.

@mfreed7
Copy link
Author

mfreed7 commented Jul 16, 2021

Yeah, the contents look good to me, but I'm pretty sure the linking needs work. Maybe it's worth landing this in different PRs assuming there's no circular dependency?

Ok, I think the linking should be pretty clean now. What do you think?

Copy link
Collaborator

@emilio emilio left a comment

Choose a reason for hiding this comment

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

This looks great, what do you think about the nit below? Either way seems fine to me.

cssom-1/Overview.bs Outdated Show resolved Hide resolved
css-cascade-5/Overview.bs Show resolved Hide resolved
@mfreed7
Copy link
Author

mfreed7 commented Jul 20, 2021

Thanks for the review/comments!

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