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

feat: open usage of data-* and aria-* attributes in CSS #632

Merged
merged 4 commits into from
Sep 12, 2018

Conversation

pmdartus
Copy link
Member

Details

Changes

  • Add support for data-* and aria-* attributes selectors.

Fix #623

Does this PR introduce a breaking change?

  • Yes
  • No

@pmdartus pmdartus requested a review from ekashida September 11, 2018 17:22
@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 5634840 | Target commit: fc9b08a

@caridy
Copy link
Contributor

caridy commented Sep 11, 2018

@pmdartus I'm fine with this.

@diervo BEST is still failing when no numbers for master are reported.

Copy link
Member

@ekashida ekashida left a 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
Copy link
Member

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. `,
Copy link
Member

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}>. `,
Copy link
Member

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}".

@pmdartus
Copy link
Member Author

@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: Invalid usage of attribute selector "${attributeName}"..

I also added a test for the aria-labelby. I am wondering what you mean by adding a test for data-, I am not sure if the compiler should be in the business of validating the selectors. As far as the compiler is concerned the data- selector is a valid data-* selector. The styling works as expected, dataset property have the right entry: https://jsfiddle.net/pmdartus/9j2pq6k8/

@salesforce-best-lwc-internal
Copy link

Benchmark results

Base commit: 5634840 | Target commit: 9bc6033

});

it('should allow usage of ARIA attributes', async () => {
await expect(process('[data-labelledby] {}')).resolves.toBeDefined();
Copy link
Member

Choose a reason for hiding this comment

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

data-labelledby => aria-labelledby

@pmdartus pmdartus merged commit 9e269bf into master Sep 12, 2018
@pmdartus pmdartus deleted the pmdartus/css-attributes branch September 12, 2018 23:37
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.

3 participants