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

[css-transitions-2] Add spec for @starting-style #8174 #8876

Merged
merged 6 commits into from
Jun 22, 2023

Conversation

lilles
Copy link
Member

@lilles lilles commented May 25, 2023

[css-transitions-2] Add spec for @starting-style #8174

@lilles lilles requested review from birtles and dbaron May 25, 2023 13:52
@tabatkins
Copy link
Member

I think the language about the global at-rules needs to be tightened up a bit. We don't have a good template text to work from yet, I'll see what I can put together.

Also, this uses a different (third!) indentation style from the rest of the document. Please use tabs for indentation, as is used elsewhere in the spec text.

@lilles
Copy link
Member Author

lilles commented May 26, 2023

I think the language about the global at-rules needs to be tightened up a bit. We don't have a good template text to work from yet, I'll see what I can put together.

I basically copied that from the container queries spec. Also, @scope needs the same specification. Wonder if we should have a common spec saying something about conditional rules that evaluate in an element context do not restrict those tree-scope-global rules.

Also, this uses a different (third!) indentation style from the rest of the document. Please use tabs for indentation, as is used elsewhere in the spec text.

Using tabs in the second commit. How does it look now?

@tabatkins
Copy link
Member

Wonder if we should have a common spec saying something about conditional rules that evaluate in an element context do not restrict those tree-scope-global rules.

Yeah, we should, now that it's getting carried to multiple places. Don't worry about it for here, tho.

How does it look now?

r+

@tabatkins
Copy link
Member

(But since I'm not a Transitions editor I'll leave it to @birtles or @dbaron to actually merge.)

css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved

<div class=example>

Transitioning an element with opacity in and out of display:none:
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it would be useful to have a few sentences of explanation on why there are two display: none; opacity: 0; pieces and what each one does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Realized display:none in the starting style was actually wrong. Corrected and commented.

Copy link
Member

Choose a reason for hiding this comment

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

OK, but I think it's probably still worth explaining what the two opacity: 0 declarations do -- and how one of them applies to the transition when class=hidden is removed and the other applies to the transition when class=hidden is added.

Copy link
Member Author

Choose a reason for hiding this comment

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

I sort of tried to do that already, and I've modified the description a bit, but I'm not quite sure how you'd want it.

Copy link
Member

Choose a reason for hiding this comment

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

OK, mostly looks good, but I left a few comments.

@dbaron dbaron self-requested a review June 14, 2023 15:34
aarongable pushed a commit to chromium/chromium that referenced this pull request Jun 16, 2023
There is no conditionText, which means we can inherit directly from
CSSGroupingRule instead of CSSConditionRule.

Result of spec PR review:

w3c/csswg-drafts#8876 (comment)

Bug: 1412851
Change-Id: Idbd174813b8894154a283885c3cfbdb35c6c9fcd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4571598
Commit-Queue: Rune Lillesveen <[email protected]>
Reviewed-by: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1158678}
Copy link
Member

@dbaron dbaron left a comment

Choose a reason for hiding this comment

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

Sorry for the delay getting back to you -- should be mostly back in my normal work location this week after visiting a bunch of folks in my team (3907km away) last week.

When you reply, I should catch the email -- but if you hit the "re-request review" button next to my username, that will slightly lower the risk that I miss it.

css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved

<div class=example>

Transitioning an element with opacity in and out of display:none:
Copy link
Member

Choose a reason for hiding this comment

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

OK, but I think it's probably still worth explaining what the two opacity: 0 declarations do -- and how one of them applies to the transition when class=hidden is removed and the other applies to the transition when class=hidden is added.

@lilles lilles requested a review from dbaron June 21, 2023 14:50
Copy link
Member

@dbaron dbaron left a comment

Choose a reason for hiding this comment

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

OK, thanks for all the iteration on this. I think this mostly looks good now, but I've left a bunch of comments on details of wording and markup. This looks good to me with those comments addressed.

css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved
css-transitions-2/Overview.bs Outdated Show resolved Hide resolved

<div class=example>

Transitioning an element with opacity in and out of display:none:
Copy link
Member

Choose a reason for hiding this comment

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

OK, mostly looks good, but I left a few comments.

@lilles lilles merged commit c9f5f54 into w3c:main Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants