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

feat(utils): Update CSSOM for nested @import computation #1339

Merged
merged 46 commits into from
May 14, 2019

Conversation

jeeyyy
Copy link
Contributor

@jeeyyy jeeyyy commented Jan 29, 2019

This PR does the below:

  • handles cssom resolution from nested @import.
    • tackles multiple level nesting @import.
    • tackles cyclic nesting via @import.
    • tackles duplication of @imports across sheets.
    • tackles both cross origin and same origin @import(s).
  • updated tests
  • using Promise instead of axe.utils.queue

Closes issue:

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: Steve

@jeeyyy jeeyyy requested a review from a team as a code owner January 29, 2019 21:56
@jeeyyy jeeyyy changed the title [WIP] refactor: update preload cssom for nested import resolution fix: update cssom for nested import resolution Jan 31, 2019
@jeeyyy jeeyyy changed the title fix: update cssom for nested import resolution fix: update cssom for nested @import resolution Jan 31, 2019
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

I think I got most of it. I've asked @stephenmathieson to work with you on refactoring this code. It is too difficult to read, which is going to make it difficult to maintain or update.

test/integration/full/preload-cssom/preload-cssom.js Outdated Show resolved Hide resolved
test/integration/full/preload-cssom/preload-cssom.js Outdated Show resolved Hide resolved
test/integration/full/preload-cssom/preload-cssom.js Outdated Show resolved Hide resolved
test/integration/full/preload-cssom/multiple-import-3.css Outdated Show resolved Hide resolved
test/integration/full/preload-cssom/multiple-import-2.css Outdated Show resolved Hide resolved
lib/core/utils/preload-cssom.js Outdated Show resolved Hide resolved
lib/core/utils/preload-cssom.js Outdated Show resolved Hide resolved
lib/core/utils/preload-cssom.js Outdated Show resolved Hide resolved
lib/core/utils/preload-cssom.js Outdated Show resolved Hide resolved
lib/core/utils/preload-cssom.js Outdated Show resolved Hide resolved
@jeeyyy jeeyyy changed the title fix: update cssom for nested @import resolution [WIP] fix: update cssom for nested @import resolution Feb 12, 2019
@jeeyyy jeeyyy closed this Feb 12, 2019
@jeeyyy
Copy link
Contributor Author

jeeyyy commented Feb 12, 2019

Refactoring of CSSOM will be done on the code in develop branch & then this PR will be re-opened.

@jeeyyy jeeyyy reopened this Mar 11, 2019
@jeeyyy jeeyyy changed the title chore: update cssom for nested @import resolution [WIP] chore: update cssom for nested @import resolution Apr 30, 2019
@jeeyyy jeeyyy changed the title [WIP] chore: update cssom for nested @import resolution chore: update cssom for nested @import resolution May 1, 2019
Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

Please add unit tests for all the new util functions that have been split out into their own files.

test/core/utils/preload.js Outdated Show resolved Hide resolved
@jeeyyy jeeyyy changed the title chore: update cssom for nested @import resolution [WIP] chore: update cssom for nested @import resolution May 1, 2019
straker
straker previously approved these changes May 2, 2019
@straker
Copy link
Contributor

straker commented May 2, 2019

@WilcoFiers needs to have final say. Changes look good to me with two caveats:

  1. The test to check if a promise is returned will break the tests in any environment where Promise is polyfilled, such as IE 11. If we're OK with that then no change is needed.
  2. The unit and integration tests for the main parse stylesheet function tests functionality as a whole set. If we're OK that the separate util functions are not tested individually then no change is needed.

@jeeyyy jeeyyy changed the title [WIP] chore: update cssom for nested @import resolution chore: update cssom for nested @import resolution May 7, 2019
@jeeyyy jeeyyy changed the title chore: update cssom for nested @import resolution [WIP] chore: update cssom for nested @import resolution May 7, 2019
@jeeyyy jeeyyy changed the title [WIP] chore: update cssom for nested @import resolution chore: Update CSSOM for nested @import computation May 13, 2019
@jeeyyy jeeyyy requested a review from straker May 13, 2019 11:15
@straker straker changed the title chore: Update CSSOM for nested @import computation feat(utils): Update CSSOM for nested @import computation May 13, 2019
@jeeyyy jeeyyy merged commit a4e177b into develop May 14, 2019
@jeeyyy jeeyyy deleted the core-preload-cssom-updates branch May 14, 2019 16:56
@scurker scurker mentioned this pull request May 15, 2019
4 tasks
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.

4 participants