Skip to content

Comments

Support content: attr() and content: counter()#559

Merged
davesnx merged 10 commits intomainfrom
support-content-attr-element
Aug 21, 2025
Merged

Support content: attr() and content: counter()#559
davesnx merged 10 commits intomainfrom
support-content-attr-element

Conversation

@davesnx
Copy link
Owner

@davesnx davesnx commented Aug 21, 2025

No description provided.

@davesnx davesnx requested a review from Copilot August 21, 2025 10:52
@vercel
Copy link

vercel bot commented Aug 21, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
styled-ppx Ignored Ignored Preview Aug 21, 2025 11:47am

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for content: attr() and content: counter() CSS properties by implementing comprehensive parsing and rendering for attribute and counter functions in CSS content declarations.

  • Extends the CSS type system to support symbols, counter styles, and attribute functions
  • Implements parsing and rendering for attr() function with optional type specifications
  • Adds counter() function support with optional counter style parameters

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/runtime/native/shared/Css_types.ml Adds new type definitions for symbols, counter styles, and enhanced attribute/counter support
packages/ppx/test/css-support/content.t/run.t Updates test expectations to include new counter() and attr() function outputs
packages/ppx/test/css-support/content.t/input.re Adds test cases for attr() and counter() CSS functions with various parameters
packages/ppx/src/Property_to_runtime.re Implements rendering functions for attribute and counter CSS functions
packages/css-property-parser/ppx/Generate.re Updates code generation to handle new CSS keywords and type mappings
packages/css-property-parser/lib/Standard.rei Updates interface to return parsed string values instead of unit types
packages/css-property-parser/lib/Standard.re Implements string token and identifier token parsing
packages/css-property-parser/lib/Parser.re Adds comprehensive grammar definitions for attribute and counter CSS functions

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

{js|symbols(|js}
^ Kloth.Array.map_and_join ~sep:{js|,|js} ~f:image_or_string_to_string
images
^ {js|)|js}
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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

The closing parenthesis is missing when symbols_type is Some. The code should append {js|)|js} in both branches of the match expression.

Copilot uses AI. Check for mistakes.
} else {
let first = str.[0];
let last = str.[length - 1];
let first = get(str, 0);
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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

[nitpick] The local variable get is unnecessary. It would be clearer to use String.get directly in the code rather than creating a local alias.

Copilot uses AI. Check for mistakes.
[%e render_list_image_or_string(~loc, list_image_or_string)],
))
]
| None => [%expr
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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

Missing tuple wrapping for the symbols variant when symbols_type is None. Should be (None, [%e render_list_image_or_string(~loc, list_image_or_string)]) to match the expected tuple structure.

Suggested change
| None => [%expr
`symbols((
None,
[%e render_list_image_or_string(~loc, list_image_or_string)],
))

Copilot uses AI. Check for mistakes.
| "upper-latin" => [%expr `upperLatin]
| "lower-roman" => [%expr `lowerRoman]
| "upper-roman" => [%expr `upperRoman]
| "none" => [%expr `none]
Copy link

Copilot AI Aug 21, 2025

Choose a reason for hiding this comment

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

The fallback case returns a string expression instead of a variant. This is inconsistent with other cases that return variants like disc, decimal, etc. Consider either removing the fallback or ensuring it returns an appropriate variant type.

Suggested change
| "none" => [%expr `none]
| _ => [%expr `Custom([%e render_string(~loc, label)])] /* fallback for unknown styles */

Copilot uses AI. Check for mistakes.
@github-actions
Copy link

github-actions bot commented Aug 21, 2025

New nightly version has been published to the NPM registry: @davesnx/styled-ppx@0.56.1-34cb3d6.0.
Install it with npm install @davesnx/styled-ppx@nightly or npm install @davesnx/styled-ppx@0.56.1-34cb3d6.0.

…yled-ppx into support-content-attr-element

* 'support-content-attr-element' of github.com:/davesnx/styled-ppx:
  Remove useles polyvars on list-style-type
@davesnx davesnx merged commit b1b2fbb into main Aug 21, 2025
9 checks passed
davesnx added a commit that referenced this pull request Aug 21, 2025
* Support attr()

* Support counter

* Support counter

* Inline ppxlib, goodbye label

* Remove old properties from ms

* Remove useless code

* Remove useles polyvars on list-style-type

* Remove useles polyvars on list-style-type
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.

1 participant