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

Add support for replacing all styles #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skovhus
Copy link
Contributor

@skovhus skovhus commented Mar 12, 2018

This adds a boolean replaceAllDefaultStyles indicating if all default styles should be overridden by the given styles property.

Useful if you want complete control of the styling.

@skovhus
Copy link
Contributor Author

skovhus commented Apr 3, 2018

@Benjamin-Dobell anything you would like me to change here?

@Benjamin-Dobell
Copy link
Owner

Hmm, I'm not 100% sold on this.

I think this particular implementation makes it a bit too easy for a user to forget to specify styles. If functionality like this is to be added, then I think strong type checking on the styles must be included with it; such that apps very loudly break when a style is missing. This is particularly important if I were to add additional styles in future releases.

The naming of the property also somewhat begs the question as to why there isn't an equivalent replaceAllDefaultRules property.

@skovhus
Copy link
Contributor Author

skovhus commented Apr 3, 2018

This is a power feature. It’s not useful for most people, but if you want complete control you need to be able to override the styles.

Alternative to this would be to make it configurable whenever a style tag would be merged on not.

Not 100% sure if that would also solve the issue right now.

@skovhus
Copy link
Contributor Author

skovhus commented Apr 3, 2018

As it is a power feature I would argue that validation isn’t super important...

@Benjamin-Dobell
Copy link
Owner

Benjamin-Dobell commented Apr 3, 2018

I wouldn't consider what you're wanting to achieve a power feature. I understand why you want it, and I think such a feature (if it exists) should be easy to use. It only comes across as a power feature because the proposed implementation is dangerous.

If you really want to be able to achieve this in your own project without worrying about style safety and what-not, then I think the best thing to do in the short-term would be to submit another pull request that simply exports DefaultRules from MarkdownView.js.

Then this can be implemented trivially in a wrapper component that simply maps each of the DefaultRules to a copy of itself with the exception of render; which would be overridden with a function that calls the original render method with styles from the wrapper component's props, rather than the provided styles parameter. Then pass these rules to a child MarkdownView and forward through this.props.children.

@skovhus
Copy link
Contributor Author

skovhus commented Apr 3, 2018

By dangerous you mean that a the use might forget to style a specific tag?

What would be the ideal solution you think?

I think it would be enable override all existing styles + Validate that the given styling object contains all tags.

@Benjamin-Dobell
Copy link
Owner

I think it would be enable override all existing styles + Validate that the given styling object contains all tags.

Agreed.

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