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] Style directives #5923

Merged
merged 39 commits into from
Jan 8, 2022
Merged

Conversation

plmrry
Copy link
Contributor

@plmrry plmrry commented Jan 24, 2021

Style Directives

Related RFC

To-Do

  • Documentation
  • Parser warnings / errors?

Code

  • In compile/nodes/Element.ts, match Style types and push styles in to styles array:
  • Create Style node, get text from info: compile/nodes/Style.ts
  • In tag.ts:
    • Allow Style node with string value
    • Add expression to directive
    • Add Style to get_directive_type
  • Add Style interface to compile/nodes/interfaces.ts
  • Add Style interface to compiler/interfaces.ts
  • Render DOM:
    • In compile/render_dom/wrappers/Element/index.ts, push updaters into hydrate chunks
    • Handle dependency updates
  • Render SSR:
    • Collect style expressions
    • Handle spread case
    • Handle case of style attribute with style directives
    • In runtime/internal/ssr.ts, create new add_styles function
    • Call add_styles with concatenated style expressions
    • Modify spread internal to handle styles

Tests

  • Parser tests:
    • With variable
    • With string
  • Render tests:
    • With variable
    • With string
    • With style attribute
    • With spread
    • With dynamic update

Cleanup

  • Remove .solo tests
  • Cleanup comments

Copy link
Member

@tanhauhau tanhauhau left a comment

Choose a reason for hiding this comment

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

keep up the work!

src/runtime/internal/ssr.ts Outdated Show resolved Hide resolved
@plmrry plmrry requested a review from tanhauhau January 27, 2021 03:01
@plmrry plmrry changed the title [WIP] Style directives Style directives Jan 27, 2021
@plmrry plmrry marked this pull request as ready for review January 27, 2021 03:06
Copy link
Member

@tanhauhau tanhauhau left a comment

Choose a reason for hiding this comment

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

Thanks for your PR, great job 👍🏻

I've left some comments on the PR and also i wonder what happens if the style: directive clashes with the style attribute?

eg:

<h1 style:font-size="30px" style="color: red; font-size: 40px;" style:color="blue" />

src/compiler/parse/state/tag.ts Outdated Show resolved Hide resolved
src/compiler/parse/state/tag.ts Outdated Show resolved Hide resolved
@plmrry
Copy link
Contributor Author

plmrry commented Mar 5, 2021

@tanhauhau @Rich-Harris Anything else this needs?

@kevmodrome
Copy link
Contributor

Are there any blockers to this being merged? Would love to use this feature! :)

@tanhauhau tanhauhau force-pushed the style-directives branch 3 times, most recently from 29f7db2 to c3b6140 Compare July 12, 2021 07:43
@tanhauhau
Copy link
Member

i've rebased the branch to the latest master and

  • fixed the ssr case where style directive and style attribute is setting the same style property
  • fixed the parsing error edge case
  • only update the style when needed

@dummdidumm
Copy link
Member

Also closes #5204 (we don't want to adjust the behavior, it's not worth documenting that small gotcha, and the OP in that issue presented exactly this style directive as a workaround)

@plmrry
Copy link
Contributor Author

plmrry commented Jul 14, 2021

i've rebased the branch to the latest master and

  • fixed the ssr case where style directive and style attribute is setting the same style property
  • fixed the parsing error edge case
  • only update the style when needed

Thank you so much @tanhauhau! Some of those changes were definitely over my head 😅

@plmrry plmrry requested a review from tanhauhau August 10, 2021 21:25
@plmrry
Copy link
Contributor Author

plmrry commented Aug 10, 2021

Gentle nudge to @benmccann @tanhauhau — any thoughts on merging this?

@benmccann
Copy link
Member

@tanhauhau / @Conduitry would have to sign off on this one. I'm not qualified 😄

@kevmodrome
Copy link
Contributor

Another gentle nudge on this 👀 Would love to start using this!

@Prinzhorn
Copy link
Contributor

The RFC mentions variables (I assume also vendor prefixes, because setProperty doesn't care).

<div style:--border-color="saddleBrown" />

I couldn't find a test for that in this PR (I did ctrl+f for style:-). Does it work?

@plmrry
Copy link
Contributor Author

plmrry commented Sep 8, 2021

I couldn't find a test for that in this PR (I did ctrl+f for style:-). Does it work?

Good point, I'll add a test

@arxpoetica
Copy link
Member

I'm a little late to the game on this, but I'd like to see camelCase variable names supported as well as pipe-case, i.e., borderRadius and border-radius should both work.

I would think this shouldn't be too much trouble; since we're a compiler, we can optimize for either case.

Why it matters: if we allow for shorthand, border-radius will never work as a shorthand variable name (for example).

@aewing
Copy link

aewing commented Nov 30, 2021

Why it matters: if we allow for shorthand, border-radius will never work as a shorthand variable name (for example).

This gap exists for shorthand attributes like aria-label, etc... already, and was pointed out in the the RFC PR, FWIW.

@plmrry plmrry changed the title Style directives [feat] Style directives Dec 2, 2021
@Rich-Harris Rich-Harris merged commit 8a47b5e into sveltejs:master Jan 8, 2022
@didier
Copy link

didier commented Jan 8, 2022

🥳🥳🥳

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.

set_style optimization silently breaks on invalid style property value
10 participants