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

Only apply svelte-123xyz attributes where necessary #680

Merged
merged 23 commits into from
Jul 5, 2017
Merged

Conversation

Rich-Harris
Copy link
Member

Making a start on #679 (and laying the groundwork for #678). Have hit a bit of a snag — the work needs to happen in preprocess, because of descendant selectors:

<div>
  <p>both this element and its parent need the scoping attribute</p>
</div>

<div>
  <strong>neither of these two need the attribute</strong>
</div>

That's going to cause some nasty merge conflicts unless I merge #673 and #676, so I'll do that first.

@Rich-Harris
Copy link
Member Author

This is almost done — think I just need to implement selectorAppliesTo for universal TypeSelector (*), ID selectors, attribute selectors, and possibly ~ and + combinators (though might just bail for now in those cases)

@Rich-Harris
Copy link
Member Author

Taking the WIP off the title, in case anyone fancies taking a look!

@Rich-Harris Rich-Harris changed the title [WIP] Only apply svelte-123xyz attributes where necessary Only apply svelte-123xyz attributes where necessary Jul 2, 2017
@@ -40,6 +41,8 @@ export default class Generator {
cssId: string;
usesRefs: boolean;

selectors: any[]; // TODO how to indicate it takes `(Node[]) => boolean` functions?
Copy link
Member

@Conduitry Conduitry Jul 2, 2017

Choose a reason for hiding this comment

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

selectors: ((nodes: Node[]) => boolean)[]; I believe

edit: Although this type doesn't seem to match how it's used later. extractSelectors returns simply Node[].

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Have refactored it a bit so that it's a bit more TS-friendly (and possibly a bit clearer anyway) — a Selector class with an apply method

@Rich-Harris
Copy link
Member Author

Did a bit more futzing around after merging #689 and concluded that we could use a more comprehensive refactoring of the CSS code, which is currently a bit scattered and woolly. In particular, it would be nice to support CSS sourcemaps (and maybe in future, CSS from multiple sources i.e. @import and <link>), which would probably be a bit of a pain to implement at the moment. Will return to that after fixing the bugs that have been raised recently.

@Rich-Harris Rich-Harris merged commit ef33466 into master Jul 5, 2017
@Rich-Harris Rich-Harris deleted the gh-679 branch July 5, 2017 16:04
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.

2 participants