Skip to content

Fix order of cascading style inheritance #133

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

Merged
merged 4 commits into from
Jul 22, 2017

Conversation

isilher
Copy link
Collaborator

@isilher isilher commented Jul 19, 2017

gettingsomewhere

Style inheritance was added in #125, but the order of applying them is reversed from the expected cascading result:

<HTMLView
  value="<b>RED<u>GREEN<i>BLUE</i></u></b>"
  stylesheet={{
    b: { color: 'red' },
    u: { color: 'green' },
    i: { color: 'blue' },
  }}
/>

screen shot 2017-07-19 at 17 46 52

This fix will add the parent styles before the child styles, fixing cascading inheritance as expected:

screen shot 2017-07-19 at 17 46 24

@bas-ie
Copy link
Collaborator

bas-ie commented Jul 22, 2017

@isilher Nice catch! Now, this might be a chance to fix something that's been bugging me... all the empty objects getting passed in to the style array. Even with your fix above, we still have three successive objects with the color prop which clutters things up a fair bit. What if instead we just did this:

  function inheritedStyle(parent) {
    if (!parent) return null;
    const style = StyleSheet.flatten(opts.styles[parent.name]) || {};
    const parentStyle = inheritedStyle(parent.parent) || {};
    return {...parentStyle, ...style};
  }

This way, every component will have just two objects passed to the style array: the default style and the custom. If there's a color prop on style, it'll overwrite parentStyle. It's only a shallow clone, but that shouldn't matter as we're not dealing with nested Sass or LESS.

If you like this, drop it in and update the snapshots (you'll need to import StyleSheet for the static flatten) and we'll be good to merge.

@isilher
Copy link
Collaborator Author

isilher commented Jul 22, 2017

@richchurcher I implemented part of your suggestion: I could not find a difference with or without the StyleSheet.flatten. Combining the object already takes care of any duplication. Or perhaps I'm missing a use case?

@bas-ie
Copy link
Collaborator

bas-ie commented Jul 22, 2017

@isilher Yeah, so the trick here is that if you can trust everyone to use plain objects as styles you'll be fine. However, the recommendation from React Native is that StyleSheets are used (and it's all through the react-native-htmlview documentation).

Without flatten, these will fail since they need to be resolved (they only compile once, to an instance of StyleSheet which is basically properties associated with a reference number). If you try to use Object.assign or object spread with one you'll get an error about trying to assign with sources other than an object.

flatten works for both ordinary objects and StyleSheets, so it's best to use it here.

Edit: I might just merge and add it on the fly, hope that's ok! Thanks again for your work 😄

@bas-ie bas-ie merged commit ab708ac into jsdf:master Jul 22, 2017
@isilher
Copy link
Collaborator Author

isilher commented Jul 23, 2017

@richchurcher I understand now, and agree StyleSheets should be supported. Thanks for explaining!

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