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

Use css-tree parser for wrapping theme styles #21742

Closed
wants to merge 7 commits into from

Conversation

strarsis
Copy link
Contributor

@strarsis strarsis commented Apr 20, 2020

Description

This PR adds the CSSTree parser for wrapping the theme styles into a styles wrapper.

How has this been tested?

I tested this on my local WordPress site.
Apparently there aren't tests (except some traversal stuff that had been only internally used anyway).

Edit:

  • What exactly is the purpose of baseUrl passed in the rules objects to the method?
  • Test added with (complex) input and expected output CSS.

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@youknowriad
Copy link
Contributor

Nice PR, I'll plan to take a look at it in the upcoming days.

@strarsis
Copy link
Contributor Author

strarsis commented Apr 26, 2020

@youknowriad: I also added a test for transforming the styles.
What is the purpose of the baseUrl parameter (that is never set in all the runs I tried with the transform function)? Is this for rewriting all style URLs in the CSS?

(The test can be run individually using this invocation: ./node_modules/.bin/jest --config test/unit/jest.config.js ./packages/block-editor/src/utils/transform-styles/test/transform-styles.js)

@youknowriad
Copy link
Contributor

youknowriad commented Apr 27, 2020

What is the purpose of the baseUrl parameter (that is never set in all the runs I tried with the transform function)? Is this for rewriting all style URLs in the CSS?

Yes, if I remember properly, it was used to transform relative and absolute URLs in CSS rules. I'm not sure how it evolved though.

@strarsis
Copy link
Contributor Author

@strarsis
Copy link
Contributor Author

strarsis commented Apr 27, 2020

@youknowriad: Based on the tests, support for baseURL has been added.
One thing though: URLs that start with a slash (e.g. /image/test.jpg) are called "absolute" URLs in the test, which is incorrect as those URLs are also relative URLs, they are just normalized ("flattened", no .. inside). The WHATWG (native) URL class is used, which can take proper care of all kinds of URLs, including data-URLs and automatically rebases and normalizes them.

@noahtallen
Copy link
Member

Bad rebase? ;)

@strarsis
Copy link
Contributor Author

@noahtallen: Yes, should I try to fix it or make a new PR?

@noahtallen
Copy link
Member

I think you should be able to fix it in the current PR. I would probably try:

  1. Locally update your fork's master branch to current Gutenberg master branch
  2. Locally, with this branch checked out, run git rebase master locally
  3. Then git push --force to get those changes to this PR

@noahtallen
Copy link
Member

Oh, when you are rebasing, do git rebase -i master to do an interactive rebase. It looks like you might need to remove commit 068183e from the PR before you push it

@strarsis
Copy link
Contributor Author

@noahtallen: Well, it turned out that some implementation details were missing. For preserving the original test structure I created a new PR that supersedes this one: #21936

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants