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

V2 css enhance #805

Closed
wants to merge 4 commits into from
Closed

V2 css enhance #805

wants to merge 4 commits into from

Conversation

cbbfcd
Copy link

@cbbfcd cbbfcd commented Feb 18, 2019

just like this:

// the props
{
  style: {
     color: 'orange !important'
  }
}

according to this 👉setProperty

⚠️:value must not contain "!important" -- that should be set using the priority paramete

and i have a test:

// it's no use
someNode.style.color = 'orange !important'

So I think there should be a test about importance here.

cbbfcd and others added 4 commits February 12, 2019 17:44
…e(null)

Object literals (e.g. { a: 3 }) automatically inherit from Object. This means that the object created by object literals will have methods defined in the Object.prototype like hasOwnProperty, toString, etc.

We could make our virtual DOM a little bit "purer" by using Object.create(null). This will create a truly plain object that doesn't inherit from Object but null instead.
just for [jorgebucaran#801](jorgebucaran#801)!

Replace the Object.assign method with a lightweight polyfill. 

Of course, this PR is still pending, and the core is still about side effects.
@codecov-io
Copy link

codecov-io commented Feb 18, 2019

Codecov Report

Merging #805 into V2 will decrease coverage by 0.38%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##               V2     #805      +/-   ##
==========================================
- Coverage   31.53%   31.14%   -0.39%     
==========================================
  Files           1        1              
  Lines         241      244       +3     
  Branches       73       74       +1     
==========================================
  Hits           76       76              
- Misses        111      113       +2     
- Partials       54       55       +1
Impacted Files Coverage Δ
src/index.js 31.14% <0%> (-0.39%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9d6757...5b42ee6. Read the comment docs.

@lukejacksonn
Copy link
Contributor

Using important is usually a last resort.. is using important on inline styles ever advised? Or are you just going for complete/closeness to the spec?

@mrozbarry
Copy link
Contributor

mrozbarry commented Feb 18, 2019

Is this necessary? Inline styles always take precedence unless a css style was already marked as !important elsewhere. Furthermore, like @lukejacksonn said, marking important styles is always a last resort, so maybe we shouldn't encourage it.

@cbbfcd Are you running into an issue where you can't use css and must flag the inline style as important?

Another thing to consider is that react has not put any stance on using !important either.

@frenzzy
Copy link
Contributor

frenzzy commented Feb 18, 2019

Have you tried to use cssText? Why don't you like it?

<div style={{ cssText: 'color:orange !important' }} />

Your approach will not work with styles in lowerCamelCase like this one:

<div style={{ fontSize: '32px !important' }} />

@cbbfcd
Copy link
Author

cbbfcd commented Feb 19, 2019

@lukejacksonn

Sorry, I did some research. My idea is indeed wrong. Thank you for your correction.

@cbbfcd cbbfcd closed this Feb 19, 2019
@cbbfcd
Copy link
Author

cbbfcd commented Feb 19, 2019

Have you tried to use cssText? Why don't you like it?

<div style={{ cssText: 'color:orange !important' }} />

Your approach will not work with styles in lowerCamelCase like this one:

<div style={{ fontSize: '32px !important' }} />

yep,

My idea is indeed wrong. thank u very much.

@cbbfcd
Copy link
Author

cbbfcd commented Feb 19, 2019

Is this necessary? Inline styles always take precedence unless a css style was already marked as !important elsewhere. Furthermore, like @lukejacksonn said, marking important styles is always a last resort, so maybe we shouldn't encourage it.

@cbbfcd Are you running into an issue where you can't use css and must flag the inline style as important?

Another thing to consider is that react has not put any stance on using !important either.

My idea is indeed wrong. thank u very much

@jorgebucaran
Copy link
Owner

@cbbfcd Could you merge or remove your duplicate comments? Thanks.

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

6 participants