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

Allow css vars with uppercase characters #2311

Merged
merged 6 commits into from Nov 26, 2018

Conversation

3 participants
@porsager
Copy link
Contributor

porsager commented Nov 26, 2018

Just noticed the fix for css vars #2308 wouldn't support uppercase characters. This fixes it.

@porsager porsager requested a review from pygy as a code owner Nov 26, 2018

@project-bot project-bot bot added this to Needs triage in Triage/bugs Nov 26, 2018

@isiahmeadows
Copy link
Collaborator

isiahmeadows left a comment

LGTM mod a couple nits.

Show resolved Hide resolved render/render.js Outdated
Show resolved Hide resolved render/render.js Outdated

porsager added some commits Nov 26, 2018

@barneycarroll

This comment has been minimized.

Copy link
Member

barneycarroll commented Nov 26, 2018

Nice catch. While we're at it, I forgot that @isiahmeadows had previously warned me about special cases: cssFloat and cssOffset. I don't know if there are others. Has that issue raised itself in BSS @porsager?

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 26, 2018

@barneycarroll It's just cssFloat (the other is just the usual offset).

@porsager

This comment has been minimized.

Copy link
Contributor

porsager commented Nov 26, 2018

@barneycarroll yeah, I've had to define float as the only property defined manually. https://github.com/porsager/bss/blob/64d8d7f3f49fe2765185fb3c3fb24a399108c180/lib/utils.js#L1

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 26, 2018

@porsager You mind filtering cssFloat to float in that loop? Also, you might want to check that your branch is up-to-date.


Off-topic: that intermediate style object in updateStyle looks incredibly redundant. IMHO the whole thing needs recast - we could probably shave a few dozen bytes in the process as well as speed up style application. Four for ... in loops and three element.style.cssText = "" seems a bit absurd when only two loops and one text reset are necessary. It's also not like we're using style as a cache or something to deduplicate with - it's just a modified clone that's generally unnecessary. It really only needs to do this:

  • If the two styles are identical, return.
  • If the new style is not an object, set elem.style.cssText to the new style (or "" if it's null/undefined) and return.
  • If the old style is not an object, set elem.style.cssText to "".
  • For each new style property without an equivalent key/value pair in the old style, set the CSS property to the value.
  • For each old style property not in the new style, remove the CSS property.

@barneycarroll I wish I caught this earlier...

Show resolved Hide resolved render/render.js Outdated
@porsager

This comment has been minimized.

Copy link
Contributor

porsager commented Nov 26, 2018

Ok, should be good now @isiahmeadows , @barneycarroll .. Let me know if you want me to take a stab at improving the function per your guidelines/code.

@isiahmeadows isiahmeadows merged commit 7c299ab into MithrilJS:next Nov 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Triage/bugs automation moved this from Needs triage to Closed Nov 26, 2018

@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 26, 2018

@porsager If you want to take a stab using my code, go for it. I really don't want to release the next release candidate without that fix, since I would rather not risk causing perf issues in users' apps.

@isiahmeadows isiahmeadows referenced this pull request Nov 27, 2018

Merged

Fix style updates to avoid unnecessary allocation #2312

7 of 11 tasks complete
@isiahmeadows

This comment has been minimized.

Copy link
Collaborator

isiahmeadows commented Nov 27, 2018

@porsager I just went ahead and used what I had (with minor alterations): #2312.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment