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

Remove abandoned value + fix renamed ones #697

Merged
merged 2 commits into from
Jan 3, 2024
Merged

Conversation

teoli2003
Copy link
Contributor

@teoli2003 teoli2003 commented Dec 28, 2023

Several features have been renamed or abandoned over the years.

Because they are listed incorrectly here, they lead to broken links in CSS/Reference.

This PR removed them and renamed <timing-function> into the <easing-function> as used everywhere.

@teoli2003 teoli2003 changed the title Remove abandonned value + fix renamed ones Remove abandoned value + fix renamed ones Dec 28, 2023
@dipikabh dipikabh self-requested a review January 2, 2024 21:52
@dipikabh dipikabh self-assigned this Jan 2, 2024
Copy link
Contributor

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

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

Thanks for these updates, @teoli2003. All changes look good. I only have one comment in the updating_css_json.md file about the animation syntax. LMK what you think.

@@ -318,7 +318,7 @@ For shorthand properties, several entries are a list of the longhand properties
```json
{
"animation": {
"syntax": "&lt;single-animation-name&gt; || &lt;time&gt; || &lt;timing-function&gt; || &lt;time&gt; || &lt;single-animation-iteration-count&gt; || &lt;single-animation-direction&gt; || &lt;single-animation-fill-mode&gt; || &lt;single-animation-play-state&gt;",
"syntax": "&lt;single-animation-name&gt; || &lt;time&gt; || &lt;easing-function&gt; || &lt;time&gt; || &lt;single-animation-iteration-count&gt; || &lt;single-animation-direction&gt; || &lt;single-animation-fill-mode&gt; || &lt;single-animation-play-state&gt;",
Copy link
Contributor

@dipikabh dipikabh Jan 2, 2024

Choose a reason for hiding this comment

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

What do you think about updating this syntax to:

Suggested change
"syntax": "&lt;single-animation-name&gt; || &lt;time&gt; || &lt;easing-function&gt; || &lt;time&gt; || &lt;single-animation-iteration-count&gt; || &lt;single-animation-direction&gt; || &lt;single-animation-fill-mode&gt; || &lt;single-animation-play-state&gt;",
"syntax": "&lt;single-animation&gt;#",

I realize that you were only updating the timing-function to the more current easing-function and I was doing a casual comparison with the syntax on the animation page when I noticed this.

The Formal syntax section and the spec list the syntax as:

animation = <single-animation>#

The value for the "syntax" key above on L321 is an enumeration of <single-animation> values. The syntaxes.json file does have an entry for single-animation. Perhaps that's the one that needs to be updated, for example, to remove <'animation-duration'> and add <time> among other things?

Also to note, in the "syntax" key on L321, &lt;time&gt; appears twice.

In this particular instance, animation was only used as an example to show how shorthand properties are treated but looks like the property syntax changed over time 🙂.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched to use border as an example of a shorthand property, as it still has the classical structure of shorthands (animation or background allow for several animations or background and use the # syntax).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a much better example. Thanks!

@teoli2003 teoli2003 requested a review from dipikabh January 3, 2024 05:51
Copy link
Contributor

@dipikabh dipikabh left a comment

Choose a reason for hiding this comment

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

Thanks for these updates!

@dipikabh dipikabh merged commit 4475a2e into mdn:main Jan 3, 2024
3 checks passed
Copy link
Contributor

github-actions bot commented Jan 3, 2024

Congratulations on your first merged pull request. 🎉 Thank you for your contribution! Did you know we have a project board with high-impact contribution opportunities? We look forward to your next contribution.

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.

2 participants