Skip to content

Conversation

@JakeQZ
Copy link
Collaborator

@JakeQZ JakeQZ commented Jan 17, 2025

Fixes #789.

Also adds an initial TestCase for Rule/Rule.

Fixes #789.

Also adds an initial `TestCase` for `Rule/Rule`.
@coveralls
Copy link

coveralls commented Jan 17, 2025

Coverage Status

coverage: 39.862% (+1.3%) from 38.583%
when pulling 11e1eb3 on bugfix/src-property
into d783f0e on main.

Copy link
Collaborator

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

Looks generally very good!

I've added some comments regarding readability.

@JakeQZ
Copy link
Collaborator Author

JakeQZ commented Jan 17, 2025

I've added some comments regarding readability.

I've made some changes and replied to those comments.

@JakeQZ JakeQZ requested a review from oliverklee January 17, 2025 18:12

declare(strict_types=1);

namespace Sabberworm\CSS\Tests\Value;
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
namespace Sabberworm\CSS\Tests\Value;
namespace Sabberworm\CSS\Tests\Unit\Value;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It lost my comment.

The namespace should also have been Rule not Value - mistake arising from copying and pasting from another file as a template.

final class RuleTest extends TestCase
{
/**
* @return array<string, array{0: string, 1: list<class-string>}>
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
* @return array<string, array{0: string, 1: list<class-string>}>
* @return array<string, array{0: string, 1: list<class-string>}>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Corrected.

@JakeQZ JakeQZ requested a review from oliverklee January 17, 2025 19:30
@oliverklee oliverklee merged commit 1312700 into main Jan 17, 2025
21 checks passed
@oliverklee oliverklee deleted the bugfix/src-property branch January 17, 2025 20:45
oliverklee pushed a commit that referenced this pull request Jan 19, 2025
Fixes #789.

Also adds an initial `TestCase` for `Rule/Rule`.

This is the 8.x backport of #790.
oliverklee pushed a commit that referenced this pull request Jan 19, 2025
Fixes #789.

Also adds an initial `TestCase` for `Rule/Rule`.

This is the 8.x backport of #790.
oliverklee pushed a commit that referenced this pull request Jan 19, 2025
Fixes #789.

Also adds an initial `TestCase` for `Rule/Rule`.

This is the 8.x backport of #790.
JakeQZ pushed a commit that referenced this pull request Jan 19, 2025
Fixes #789.

Also adds an initial `TestCase` for `Rule/Rule`.

This is the 8.x backport of #790.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multiple values in src property of @font-face rule not parsed correctly

4 participants