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

Updated markers.ts to employ the '⚡️' prefix for component styles, enhancing consistency with the Qwik Framework. #7248

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

sreeisalso
Copy link
Contributor

What is it?

  • Feature / enhancement

Description

created PR as per discord discussion https://discord.com/channels/842438759945601056/1326121240074780722/1327350404635168852

Copy link

changeset-bot bot commented Jan 12, 2025

🦋 Changeset detected

Latest commit: e99a0fb

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

This PR includes changesets to release 4 packages
Name Type
@builder.io/qwik Patch
eslint-plugin-qwik Patch
@builder.io/qwik-city Patch
create-qwik Patch

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

Copy link

pkg-pr-new bot commented Jan 12, 2025

Open in Stackblitz

npm i https://pkg.pr.new/@builder.io/qwik@7248
npm i https://pkg.pr.new/@builder.io/qwik-city@7248
npm i https://pkg.pr.new/eslint-plugin-qwik@7248
npm i https://pkg.pr.new/create-qwik@7248

commit: e99a0fb

@sreeisalso sreeisalso marked this pull request as ready for review January 12, 2025 10:49
@sreeisalso sreeisalso requested a review from a team as a code owner January 12, 2025 10:49
@Shane-Donlon
Copy link
Contributor

Hi Everyone,

While this Change is for an icon, and doesn't require a docs Change in itself..

I do think there's also an improvement opportunity for the Docs.

This change was sparked by the question

'What does the star icon represent when inspecting the DOM?' (See Discord link in description)

Inferring from the conversation it seems that this is not clear what the purpose of the emoji is.

While the change in icon should make it more intuitive that it is intentionally added by Qwik I do think we should take the opportunity in this PR to address the initial question too.

For clarity on those that don't know who I am.

I am not from core leadership, but rather offering a question here.

I'll defer the decision on docs change in this PR to Core Leadership, and if it is needed in a separate PR that's okay too. But that second PR will be a dependency on this PR.(As the icon is changing)

Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

Actually I think it should not interpolate the prefix in the tests, so that it's easy to see exactly what is being rendered (just like before).

Other than that, it's a theoretically breaking change because some people might be using CSS with those selectors, so let's change the changeset to minor instead of patch

@wmertens wmertens added the COMP: DX Developer Experience related issue label Jan 13, 2025
@wmertens
Copy link
Member

as for the docs, yes, let's add a little explanation to the useStyles$ hook to indicate that it's done to clearly show it's generated by Qwik and to prevent clashes.

@shairez
Copy link
Contributor

shairez commented Jan 14, 2025

@Shane-Donlon yeah good idea! feel free to add a PR for the docs

@shairez
Copy link
Contributor

shairez commented Jan 14, 2025

@wmertens if this has a potential to be a breaking change (although slim), I suggest - let's add it to V2 instead of V1 (it's not a critical bug fix, just a fun change).

WDYT @sreeisalso ?

@sreeisalso
Copy link
Contributor Author

I'm just curious and investigating the Qwik source code. I came across a PR request on Discord and made the necessary change. I'm open to whatever the core team decides.

@sreeisalso
Copy link
Contributor Author

Actually I think it should not interpolate the prefix in the tests, so that it's easy to see exactly what is being rendered (just like before).

@wmertens but this will make difficult to change the marker, what will be the other option? can I define const in each test file instead of import?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
COMP: DX Developer Experience related issue
Projects
Status: Waiting For Review
Development

Successfully merging this pull request may close these issues.

4 participants