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

Improve core CSS parser. #129

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

stoivo
Copy link

@stoivo stoivo commented Nov 10, 2021

It's still early in development. I have started and wanted to share my progress in case you have some imitate feedback.

Also I have some questions
With the existion parser did you but any thouth into what you want to happend to comments? is it ok if we parse them and remove them from the return value?

You can have nested @media queries in CSS3 like below, it is valid but not recomended, it is ok not to parse it?

@media screen {
  @media (min-width: 1px) {
    @media (min-height: 1px) {
      @media (max-width: 9999px) {
        @media (max-height: 9999px) {
          body {
            background: red;
          }
        }
      }
    }
  }
}

The direction with a StringScanner, does it look ok? It can still be clean up and refactored a but. Also what would you like to name sucha a thing is css_parser? CssParser::Parser::Parser isn't greate

Pre-Merge Checklist

  • CHANGELOG.md updated with short summary

@grosser
Copy link
Contributor

grosser commented Nov 13, 2021

  • I don't see any code changes
  • never thought/cared about comments, if there are no tests for them feel free to drop ... and even if there are tests but they are hard to implement just drop 🤷
  • nested @media queries would be nice to support if they were previously supported, but feel free to drop since it seems like an edge-case

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.

None yet

2 participants