-
Notifications
You must be signed in to change notification settings - Fork 402
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
feat: open usage of data-* and aria-* attributes in CSS #632
Conversation
Benchmark resultsBase commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add test cases for [data-]
(invalid) and [aria-labelledby]
(valid)? I'm not sure if these changes will handle those as expected.
// The lookup stop when either a tag is found or when reaching the next | ||
// combinator - which indicates the end of the compound selector. | ||
// If the attribute name is not a globally available attribute, the attribute selector is required | ||
// to be associated with a tag selector, so we can validate it's usage. Let's walk the compound selector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it's
=> its
// the compound selector need to be more specific. | ||
if (tagSelector === undefined) { | ||
const message = [ | ||
`Attribute selector "${node}" is too generic. `, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Add a tag selector to validate the usage of the attribute selector" doesn't sound quite right because it sounds vague without the right context. What do you think about:
For validation purposes, attributes that are not global attributes must be associated with a tag name when used in a CSS selector (e.g., "input[min]" instead of "[min]").
const message = [ | ||
`Attribute selector "${node}" can't be applied to match on <${tagSelector}>. `, | ||
`Attribute selector "${node}" can't be applied to match on <${tagSelector.value}>. `, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about something like:
"${tagName}${node}" is an invalid selector because "${attributeName}" is not a known attribute of "${tagName}".
@ekashida I updated the wording, let me know what you think. I deviated a little from your proposal for consistency purposes. All the error start with the same sentense: I also added a test for the |
Benchmark resultsBase commit: |
}); | ||
|
||
it('should allow usage of ARIA attributes', async () => { | ||
await expect(process('[data-labelledby] {}')).resolves.toBeDefined(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data-labelledby
=> aria-labelledby
Details
Changes
data-*
andaria-*
attributes selectors.Fix #623
Does this PR introduce a breaking change?