Skip to content

Conversation

@crob611
Copy link
Contributor

@crob611 crob611 commented Oct 8, 2021

Fixes #100639

This switches from using style-it to using emotion for our CSS on the workpad and workpad elements.

See the linked issue for an example of CSS that style-it would leak that is fixed by this issue.

Also, this removes a few places where workpad.css was being added that was not necessary.

Appreciate any testing of edge cases that you can think of here.

@crob611 crob611 added review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// loe:small Small Level of Effort v8.0.0 release_note:skip Skip the PR/issue when compiling release notes impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Feature:Canvas auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Oct 8, 2021
@crob611 crob611 requested a review from a team as a code owner October 8, 2021 15:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@crob611
Copy link
Contributor Author

crob611 commented Oct 12, 2021

@elasticmachine merge upstream

@kibanamachine

This comment was marked as outdated.

@crob611
Copy link
Contributor Author

crob611 commented Nov 1, 2021

@elasticmachine merge upstream

@spalger spalger added v7.17.0 and removed v7.16.0 labels Dec 7, 2021
@markov00
Copy link
Member

markov00 commented Feb 9, 2022

@crob611 I was just passing by intrigued by the style-it library and checking the code changes it looks like we can also remove the library from the package.json file if we replace also the canvas/shareable_runtime/components/rendered_element.tsx file what do you think?

@elastic elastic deleted a comment from kibanamachine Feb 10, 2022
@crob611
Copy link
Contributor Author

crob611 commented Mar 7, 2022

@elasticmachine merge upstream

@crob611
Copy link
Contributor Author

crob611 commented Mar 7, 2022

@markov00 Good catch. Removed that usage and removed style-it package all together.

@crob611 crob611 requested a review from a team as a code owner April 7, 2022 15:50
@azasypkin
Copy link
Member

Hey @crob611,

Do you still plan to move forward with this PR? Are there any blockers or it just fell under the radar?

@spalger
Copy link
Contributor

spalger commented Jun 24, 2022

@crob611 in c778406 (#114393) I removed the @emotion/babel-preset-css-prop in the jest transform. Do you remember why you added it? It seems that was the thing that caused rendered CSS to be included in the jest snapshots which is why all those snapshots broke.

@crob611
Copy link
Contributor Author

crob611 commented Jun 24, 2022

@spenceralger

If a jest snapshot has a component using an emotion css prop and function like <div css={css(something)} />, then the output of the snapshot will have

<div css=\\"You have tried to stringify object returned from \`css\` function. It isn't supposed to be used directly (e.g. as value of the \`className\` prop), but rather handed to emotion so it can handle it (e.g. as value of \`css\` prop).

with the transform it will be something like

<div class="css-1xkabg0" />

I think that this PR is fine without it, and am not sure why I added the transform in the first place, but at some point we likely will need to add some kind of transform that will also output the css inline also?

@spalger
Copy link
Contributor

spalger commented Jun 27, 2022

@crob611 Yeah, that's not great but printing out all the rendered CSS isn't great either in my opinion. It would be ideal if it just printed the variables which were passed so that we can verify that computations in the component are valid, but without including tons of unnecessary and internal implementation details about how style variables are transformed into CSS. Either way, I think that @elastic/eui-design is probably the right team to handle that implementation since they're responsible for the emotion integration.

(btw I'm @spalger, I used to use @spenceralger but Github let someone else register it...)

@thompsongl
Copy link
Contributor

@crob611 Using @emotion/css instead of @emotion/react in rendered_element.tsx resolves the snapshot output issue. I'm not quite sure how to test this in a dev environment, though. Does using @emotion/css there retain in the correct functionality and styling?

@ThomThomson
Copy link
Contributor

ThomThomson commented Jul 4, 2022

@thompsongl, I updated it to use emotion/css as requested and it does resolve the snapshot output! I also looked at the demo workpads for flights and ecommerce, and didn't notice any regressions, but let's see if CI Passes!

Edit
looks like there are more snapshots that need replacing, and that the class names in the snapshots now have generated IDs. I'm not totally sure about this, but wasn't there recently something changed to make EUI not generate IDs like this? Should we commit these snapshot changes?

@ThomThomson
Copy link
Contributor

buildkite test this

@ThomThomson ThomThomson added v8.4.0 and removed v8.2.0 labels Jul 6, 2022
@ThomThomson
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@cqliu1 cqliu1 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I tested by uploading a bunch of old workpads with lots of custom CSS, looking through the pages and page previews, and generating reports. It looks like all the custom styles have been preserved and only apply to the workpad and nested elements.

The only styles that I tested that broke from this change are styles that come from @import statements that were previous allowed with style-it but no longer work with emotion, so things like imported Google fonts are no longer usable in Canvas workpads.

I also loaded up the workpad listed in the #100639, and the problematic element no longer appears outside of the workpad to cover parts of the global top bar.

One bug I did encounter, which doesn't necessarily need to be addressed in this PR, is I uploaded an old workpad (probably from late 6.x, early 7.x) that didn't have a css property on the workpad JSON, and that threw this error from failing the workpad schema validation on upload. It looks like css is a required property when it should probably be optional.
Screen Shot 2022-07-08 at 5 48 11 PM

@ThomThomson
Copy link
Contributor

@cqliu1 Makes sense that we can fix the loading of older workpads which are missing the css property in a follow-up. Shouldn't be too difficult! Thank you for testing this so thoroughly! I will fix the conflicts and try to get this merged today.

@ThomThomson
Copy link
Contributor

buildkite test this

@ThomThomson
Copy link
Contributor

buildkite test this

@kibana-ci
Copy link

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
canvas 1141 1140 -1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
canvas 1.0MB 1.0MB -5.9KB

Canvas Sharable Runtime

The Canvas "shareable runtime" is an bundle produced to enable running Canvas workpads outside of Kibana. This bundle is included in third-party webpages that embed canvas and therefor should be as slim as possible.

id before after diff
module count 4969 4971 +2
total size 7.7MB 7.7MB -4.1KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ThomThomson ThomThomson merged commit f128db1 into elastic:main Jul 19, 2022
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Deprecated - use backport:version if exact versions are needed backport:skip This PR does not require backporting Feature:Canvas impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v8.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Canvas: CSS can leak out of workpad