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

Allow for passing in arbitrary styles #38

Closed
conrad-vanl opened this issue Jan 7, 2018 · 14 comments
Closed

Allow for passing in arbitrary styles #38

conrad-vanl opened this issue Jan 7, 2018 · 14 comments
Labels

Comments

@conrad-vanl
Copy link

@conrad-vanl conrad-vanl commented Jan 7, 2018

Would there be any interest for a PR that added the feature to be able to pass in arbitrary styles to placeholders? We've found the built-in props for a few of our use cases to be limiting, especially for fine-tuning the positioning of placeholder elements. We'd hate to wrap the placeholders in additional views, so we're left with having to build custom placeholders. However, if these components allowed for passing in arbitrary styles for more control, we'd be able to solve for our use case.

I'm happy to open a PR if there's interest.

<Box style={...} />
<ImageContent style={...} paragraphStyle={...} mediaStyle={...} />
<Line style={...} />
<Media style={...} />
<MultiWords style={...} wordStyle={...} />
<Paragraph lineStyle={...} style={...} />
@burkeshartsis

This comment has been minimized.

Copy link

@burkeshartsis burkeshartsis commented Jan 8, 2018

A great example of this was with needing a Placeholder.Media or Placeholder.Box that respected aspect ratio. This is trivial if you could pass in the aspectRatio style prop but the current components don't allow for that.

Positioning typography components as mentioned @conrad-vanl is also unnecessarily brittle. Requiring a wrapper component, and handful of duplicated styles.

Love the library and where it's headed. A style prop seems like an obvious add to me.

@mfrachet

This comment has been minimized.

Copy link
Owner

@mfrachet mfrachet commented Jan 9, 2018

This sounds to be a good idea 😄

@mfrachet

This comment has been minimized.

Copy link
Owner

@mfrachet mfrachet commented Jan 19, 2018

Let me know if you would like to take the point or if I create a pull request from my side 😄

@conrad-vanl

This comment has been minimized.

Copy link
Author

@conrad-vanl conrad-vanl commented Jan 19, 2018

Hey @mfrachet! Apologies, since writing the first message, my wife and I gave birth to our first son, and so I've been away from work for a while. I had started this work in this branch before - https://github.com/conrad-vanl/rn-placeholder/tree/style-prop

I believe this might be pull-requestable, and happy to go ahead and open if you like. But might need a little more polish as well.

@mfrachet

This comment has been minimized.

Copy link
Owner

@mfrachet mfrachet commented Jan 20, 2018

Congrats to you and your wife 😄 I wish you all the best !

Don't worry, take time to rest and enjoy life. I will take some time to check what you've made and probably open something.

If you prefer to open the PR, I would be happy to help you on it :). Else, I'll make it mentioning your investment on the topic 😄

Have a great week end and again, congratulations 🎉

@jaulz

This comment has been minimized.

Copy link

@jaulz jaulz commented May 1, 2018

Could anyone spend some time on this? :-)

@mfrachet

This comment has been minimized.

Copy link
Owner

@mfrachet mfrachet commented Jun 1, 2018

I'm currently working on, here's the current state:

  • <Box style={...} />
  • <ImageContent style={...} paragraphStyle={...} mediaStyle={...} />
  • <Line style={...} />
  • <Media style={...} />
  • <MultiWords style={...} wordStyle={...} />
  • <Paragraph lineStyle={...} style={...} />

These are availables thanks to: https://github.com/mfrachet/rn-placeholder/pull/45/files#diff-d69027359d042f61d170dd9c0b8d851cR4

feel free to review the PR to improve the code quality 😊

@mfrachet mfrachet added the enhancement label Jun 9, 2018
@luweiCN

This comment has been minimized.

Copy link

@luweiCN luweiCN commented Feb 19, 2019

I am wondering how is it going? I am having this issue too. I just roughly edit the node_modules code to solve the problem. I hope these feature will be supported officially soon.

// placeholderContainer.js
const renderAnimation = (Animation, Component, props) => {
  if (!Animation) {
    throw new Error(`${Animation.name} doesnt exist in the current project`);
  }
  return (
    <Animation style={props.style}>
      <Component {...props} />
    </Animation>
  );
};
// animation/fade.js
return <Animated.View style={[style, { opacity: animation }]}>{children}</Animated.View>;
@mfrachet

This comment has been minimized.

Copy link
Owner

@mfrachet mfrachet commented Feb 19, 2019

Hello @luweiCN 😊

This issue concerns the different primitives of the app: it means that I will / would probably work on adding custom styles only on the component listed above.

The part you're describing is not part of this issue for now. If you need some specific behaviours concerning the animation part, you can create your own one using https://mfrachet.github.io/rn-placeholder/customize/animation.html

I hope this answer is not frustrating to your side and would be happy to discuss a bit more on a way to make the style passing between components more easy 😞

@mfrachet

This comment has been minimized.

Copy link
Owner

@mfrachet mfrachet commented Mar 19, 2019

I'm sorry for being that late dealing this issue, my new job doesn't really for open source work some hours a week so...

But I think that I come with a solution that would help this library being simpler (in term of tree level) more customizable and composable: compound components.

Let's image a V2 that would look like:

const Component = () => (
  <Placeholder onReady={this.state.isReady} animate="fade">
     <Media position="left" />
     <Line width="80%" />
     <Line width="30%" />
  </Placeholder>
)

It would allow to customize the component style directly without passing props down the tree

It would allow to create custom patterns for line and (potentially) to animate any element depending the developer needs.

With this approach, I'm also looking at a way to make the animation better without the effect it has for now (today, animation is clearly a trick)

@andyr6381

This comment has been minimized.

Copy link

@andyr6381 andyr6381 commented Mar 20, 2019

Hi thanks for the module, I'm a bit confused on the above but.

Is it possible to change the backgroundcolor and width of the shine without creating a custom component? The current shines doesn't work for dark themes. I can modify the shine.js to get what I need but can I pass these styles from the component?

const styles = StyleSheet.create({
shine: {
width: 30,
position: 'absolute',
height: '100%',
width: '100%', - changed
backgroundColor: 'black' - changed
opacity: 0.4,
},
});

@mfrachet

This comment has been minimized.

Copy link
Owner

@mfrachet mfrachet commented Mar 20, 2019

Yes, the actual shine animation is a tweak, as mentioned above. I think that for dark themes, you should make the color over the animation darker than the line color. It should make something better with this.

@mfrachet

This comment has been minimized.

Copy link
Owner

@mfrachet mfrachet commented Apr 27, 2019

I've been working on a v2 for the library that is actually evolving here:

https://github.com/mfrachet/rn-placeholder/tree/compound

This should allow to customize a bit more the placeholders and passing styles at the level we want.

Concerning the placeholder on darkmode, I may have some ideas. But it will probably take some time to investiguate 😓

@mfrachet

This comment has been minimized.

Copy link
Owner

@mfrachet mfrachet commented Apr 28, 2019

I close this issue concerning the style passing since composition has been shipped in v2 (https://github.com/mfrachet/rn-placeholder/tree/v2.0.0)

Feel free to reopen if you need more informations 😊

@mfrachet mfrachet closed this Apr 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.