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: new attribute scope style strategy #7893

Merged
merged 7 commits into from
Aug 8, 2023

Conversation

ematipico
Copy link
Member

@ematipico ematipico commented Aug 1, 2023

Changes

This adds a new scope style strategy called attribute.

When enabled, styles are emitted using the data-* attributes.

This PR changes the default value of scopedStyleStrategy, its new value is now "attribute".

Testing

Added new test cases and updated the current ones.

Docs

/cc @withastro/maintainers-docs for feedback!

@changeset-bot
Copy link

changeset-bot bot commented Aug 1, 2023

🦋 Changeset detected

Latest commit: d0f5d74

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Aug 1, 2023
@ematipico ematipico force-pushed the feat/attribute-style-strategy-plt-742 branch from 869650a to b187935 Compare August 1, 2023 10:06
@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Aug 1, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Maybe we can also release this in 2.10? But I'm fine either ways.

@matthewp
Copy link
Contributor

matthewp commented Aug 1, 2023

I think we're still bikeshedding this. If there's resolution before 2.10 we can ship it, otherwise 3.0 is fine.

@ematipico ematipico force-pushed the feat/attribute-style-strategy-plt-742 branch 2 times, most recently from 160bcb8 to 9c9882d Compare August 4, 2023 09:01
packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
packages/astro/src/@types/astro.ts Show resolved Hide resolved
@github-actions github-actions bot added the feat: markdown Related to Markdown (scope) label Aug 4, 2023
@ematipico ematipico force-pushed the feat/attribute-style-strategy-plt-742 branch 2 times, most recently from 1a7439e to d9538f1 Compare August 4, 2023 10:32
@github-actions github-actions bot removed the feat: markdown Related to Markdown (scope) label Aug 4, 2023
Comment on lines +44 to +52
for (const [key] of Object.entries(classes[0].attribs)) {
if (/^data-astro-cid-[A-Za-z0-9-]+/.test(key)) {
// Ema: this is ugly, but for reasons that I don't want to explore, cheerio
// lower case the hash of the attribute
scopedAttribute = key
.toUpperCase()
.replace('data-astro-cid-'.toUpperCase(), 'data-astro-cid-');
}
}
Copy link
Member

Choose a reason for hiding this comment

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

IIRC the HTML spec have attributes all lowercased by default, which is why it uses kebab-case. Maybe we should consider making our hashes lower-case 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

since this is mostly for aesthetically I agree here too, lowercase looks cleaner since the data- part is lower (subjective opinion of course).

Copy link
Contributor

Choose a reason for hiding this comment

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

Although this change doesn't change the hash does it? In which case I don't think that's a blocker for this issue.

Copy link
Member

Choose a reason for hiding this comment

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

That’s a very good point. In the browser they’ll be normalized to be all lowercase anyway, so we might as well generate them like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you. Worth considering for a next PR then :)

packages/astro/test/0-css.test.js Outdated Show resolved Hide resolved
@ematipico ematipico force-pushed the feat/attribute-style-strategy-plt-742 branch from 6e3df39 to d0f5d74 Compare August 8, 2023 11:12
@ematipico ematipico merged commit 7bd1b86 into next Aug 8, 2023
13 of 14 checks passed
@ematipico ematipico deleted the feat/attribute-style-strategy-plt-742 branch August 8, 2023 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants