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

fix: Fix forwarding of the style attribute #2453

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

NicholasBoll
Copy link
Member

@NicholasBoll NicholasBoll commented Dec 7, 2023

Summary

Fixes: #2452

The style attribute was being converted into a class name. The styles were being added and passing visual tests, but doing so in an unoptimized way. We use the style attribute for styles that change frequently to avoid expensive Style recalculations and we accidentally broke that.

Release Category

Components


Checklist

For the Reviewer

  • PR title is short and descriptive
  • PR summary describes the change (Fixes/Resolves linked correctly)
  • PR Release Notes describes additional information useful to call out in a release message or removed if not applicable
  • Breaking Changes provides useful information to upgrade to this code or removed if not applicable

Where Should the Reviewer Start?

Areas for Feedback? (optional)

  • Code
  • Testing

Testing Manually

You could try adding a style attribute to any example. Without this change, the style attribute gets removed and the styles get merged into the element's main CSS class.

@NicholasBoll NicholasBoll added the ready for review Code is ready for review label Dec 7, 2023
@@ -1,8 +1,8 @@
import React from 'react';
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove the React dependency. It was unintentional.

const returnProps = csToProps([localCs, className, cs, style]);
const returnProps = csToProps([localCs, className, cs]);
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding style to the csToProps was always weird. But before we added support for static inline styles, it worked. Afterwards, csToProps couldn't differentiate between a style prop given from elemProps or one given by a createVars function or a Stencil. So I removed style from the array and merge it below instead.

Copy link

cypress bot commented Dec 7, 2023

1 flaky test on run #6511 ↗︎

0 964 3 0 Flakiness 1
⚠️ You've recorded test results over your free plan limit.
Upgrade your plan to view test results.

Details:

Merge 77ad882 into 8ac4355...
Project: canvas-kit Commit: cedcb8b54e ℹ️
Status: Passed Duration: 16:00 💡
Started: Dec 7, 2023 10:39 PM Ended: Dec 7, 2023 10:55 PM

Review all test suite changes for PR #2453 ↗︎

@NicholasBoll NicholasBoll added automerge and removed ready for review Code is ready for review labels Dec 7, 2023
@alanbsmith alanbsmith merged commit 22a452c into Workday:master Dec 7, 2023
32 of 34 checks passed
@NicholasBoll NicholasBoll deleted the fix/style-attribute branch December 7, 2023 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants