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-counter-styles] Add prefix and suffix tests #11886

Closed

Conversation

xfq
Copy link
Contributor

@xfq xfq commented Jul 10, 2018

@xfq xfq added the wg-css label Jul 10, 2018
@xfq xfq requested a review from r12a July 10, 2018 12:34
@wpt-pr-bot wpt-pr-bot requested review from svgeesus and tabatkins July 10, 2018 12:34
@upsuper
Copy link
Member

upsuper commented Jul 11, 2018

@xfq
Copy link
Contributor Author

xfq commented Jul 11, 2018

Ah, I didn't know that directory. IIUC it has already covered the tests in my PR...

@upsuper
Copy link
Member

upsuper commented Jul 12, 2018

I guess we can close this then? I don't see much value adding another set of tests for the same functionality.

@xfq
Copy link
Contributor Author

xfq commented Jul 12, 2018

I guess we can close this then? I don't see much value adding another set of tests for the same functionality.

Fine with me.

@xfq xfq closed this Jul 12, 2018
</div>
<!--Notes:
You will need an appropriate font to run this test.
To see the ASCII decimal number associated with a row, mouse over it and the number will pop up in a tooltip.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is wrong - there's only a title on the ref, not the test.

Copy link
Contributor

@tabatkins tabatkins left a comment

Choose a reason for hiding this comment

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

Comments on first test apply equally to every single other test, as far as I can tell.

That said, the tests look reasonable to me otherwise.

<ol start="10"><li><span class="blank">x</span></li></ol>
<ol start="12"><li><span class="blank">x</span></li></ol>
<ol start="222"><li><span class="blank">x</span></li></ol>
<ol start="9999"><li><span class="blank">x</span></li></ol>
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly odd that this is testing additive itself at the same time as testing the default suffix. Surely it only needs one line?

<ol start="9999"><li><span class="blank">x</span></li></ol>
</div>
<!--Notes:
You will need an appropriate font to run this test.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this test using characters with spotty font coverage? That's not what this is trying to test in the first place; it seems this could be rewritten to just use ASCII.

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.

5 participants