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

Web-components: Fix CSS class usage in CLI template #17702

Merged
merged 2 commits into from Mar 14, 2022
Merged

Web-components: Fix CSS class usage in CLI template #17702

merged 2 commits into from Mar 14, 2022

Conversation

wesleyboar
Copy link
Contributor

@wesleyboar wesleyboar commented Mar 13, 2022

Issue: Styles were not being applied to an element using attribute className.

What I did

Change .js file's lit-html markup to use class attribute instead of className.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation? — Maybe.

If your answer is yes to any of these, please make sure to include it in your PR.

No "Jest or Chromatic screenshots". I tried. Storybook I failed: #17703

Edit: I found Chromatic runs automatically. It is recorded as having passed, and though I agree, I think it is a broken test.1

Update: I was ignorant of the complexity and feature set of the Storybook pipelines. See #17702 (comment).

Footnotes

  1. I found the relevant test suite ("regression-test-webcomponents"). The relevant stories ("Example" > "Page") do not reflect my change. I see classname is still used, and styles are still not applied. I don't understand the purpose of this automation.

Styles are applied to the element when the attribute is `class`, not `className`.
Mimic the attribute change made and tested on the respective `.js` file:
2daf62c
@nx-cloud
Copy link

nx-cloud bot commented Mar 13, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f1d39bc. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@wesleyboar
Copy link
Contributor Author

I see two failures for automated testing. I do not claim responsibility for those failures.

  • "ci/circleci" Details
    • Mentions Vue and ID attribute, neither of which does my code appear to influence.
  • "test" Details
    • Is inaccessible to me. I have no login.

@wesleyboar wesleyboar marked this pull request as ready for review March 13, 2022 21:19
Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

Hey @tacc-wbomar thanks for your contribution!

Sorry about the pipeline failures, we are facing an issue with E2E tests with pnp for Vue and React, and that is not related to your changes. Thanks for checking!

@yannbf yannbf added cli patch:yes Bugfix & documentation PR that need to be picked to main branch labels Mar 13, 2022
@yannbf
Copy link
Member

yannbf commented Mar 13, 2022

I found the relevant test suite ("regression-test-webcomponents"). The relevant stories ("Example" > "Page") do not reflect my change. I see classname is still used, and styles are still not applied. I don't understand the purpose of this automation.

The change you made is related to CLI templates used to bootstrap new projects. In Chromatic we use the kitchen sink app which is not related to the change you made, hence it didn't introduce changes. It's all good!

@shilman shilman added the bug label Mar 14, 2022
@shilman shilman changed the title Web Comp. Example Page, Fix Class Attribute Web-components: Fix CSS class usage in CLI template Mar 14, 2022
@shilman shilman merged commit 3702ee0 into storybookjs:next Mar 14, 2022
@wesleyboar
Copy link
Contributor Author

Thank you.

I've injected "Update: " statements into this PR desc and my issue that will clarify my inaccuracy to others and point to your comments as resolutions.

@wesleyboar wesleyboar deleted the patch-1 branch March 15, 2022 02:55
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Apr 1, 2022
shilman added a commit that referenced this pull request Apr 1, 2022
Web-components: Fix CSS class usage in CLI template
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cli patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch web-components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants