juiceo / react-native-easy-markdown

Simple & customizable React Native component to render Github-flavoured markdown using minimal native components.
https://www.npmjs.com/package/react-native-easy-markdown
65 stars 62 forks source link

Text elements wrapping unexpectedly #18

Open doug-sheridan opened 5 years ago

doug-sheridan commented 5 years ago

Is there a reason on line 215 of index.js the nodes are not wrapped in a Text?

<Text>{nodes}</Text>

Without this, every time a new styled text element is encountered a line break will occur. This is not the desired behavior.

The desired behavior should be: Styled elements such as bold and italic should not line break unless the new line character is encountered.

juiceo commented 5 years ago

Certainly not the desired behavior. Looking at the code, line 215 doesn't seem to be what you're referring to though - which function is it?

A good while back there were some issues with View components being rendered within Text components and causing crashes on Android (might be supported in a newer version of RN, but probably undesired behavior regardless), so that might be the reason for this.

Have you tested changing the code like that? I'm not currently using markdown in any of my own projects, so if you want to play around with it and submit a PR that would be much appreciated, and would probably be the fastest way to get the issue resolved. Else it might take a while for me to get around to working on it.

zivester commented 5 years ago

I believe I just ran into this bug as well. It has to do with content following a bold/italic element (but its not related to the style of those items). I believe it actually has to do with flexbox and the container wrapping the items. A simplified version of the css exhibiting this bug can be found here: https://codepen.io/anon/pen/RqmPGx

I'm currently digging into a solution for it, while remaining to use flexbox. @dsherida did you end up getting anywhere with this (possibly in a fork)?

doug-sheridan commented 5 years ago

I believe I just ran into this bug as well. It has to do with content following a bold/italic element (but its not related to the style of those items). I believe it actually has to do with flexbox and the container wrapping the items. A simplified version of the css exhibiting this bug can be found here: https://codepen.io/anon/pen/RqmPGx

I'm currently digging into a solution for it, while remaining to use flexbox. @dsherida did you end up getting anywhere with this (possibly in a fork)?

Yes I made one change to index.js from this package and that fixed my issue. You should be able to do the same. I just wrapped the {nodes} in a Text and it seemed to fix my problem. Replace the renderBlock method inside your node_modules folder with this, and it should work?

renderBlock(node, key, extras) {
        const { styles } = this.state;

        let style = [styles.block];
        let isBlockQuote;
        if (extras && extras.blockQuote) {
            isBlockQuote = true;

            /* Ensure that blockQuote style is applied only once, and not for
             * all nested components as well (unless there is a nested blockQuote)
             */
            delete extras.blockQuote;
        }
        const nodes = this.renderNodes(node.props.children, key, extras);

        if (isBlockQuote) {
            style.push(styles.blockQuote)
            return (
                <View key={'blockQuote_' + key} style={[styles.block, styles.blockQuote]}>
                    <Text>{nodes}</Text>
                </View>
            );
        }
        else {
            return (
                <View key={'block_' + key} style={styles.block}>
                    <Text>{nodes}</Text>
                </View>
            );
        }
    }
zivester commented 5 years ago

That works great, although I'm curious as to why that works. That does seem to be more consistent with the isBlockQuote line above. I wonder if it was intentional to not have it wrapped in a Text.

I'm going to dig some more, but thanks for the quick response!

Oh I see how it fixes it. It effectively changes the wrapper to display: block; which does remove the issues presented by display: flex as shown in the fiddle.

doug-sheridan commented 5 years ago

Do let me know what you find. We’ve been using this “hack” in production with seemingly no issues… best of luck mate

On Dec 4, 2018, at 10:40 AM, zivester notifications@github.com wrote:

That works great, although I'm curious as to why that works. That does seem to be more consistent with the isBlockQuote line above. I wonder if it was intentional to not have it wrapped in a Text.

I'm going to dig some more, but thanks for the quick response!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lappalj4/react-native-easy-markdown/issues/18#issuecomment-444189596, or mute the thread https://github.com/notifications/unsubscribe-auth/AFKaOM6FyScjmOI6VzfLQ6yNSIXe4A64ks5u1rOAgaJpZM4WAilm.

zivester commented 5 years ago

I submitted a PR #19 with your fix. It seems to be working well for us so far on mobile and with react-native-web. I can't think of any issues for our use case that will arise. Maybe @lappalj4 can let us know if there was a reason for the absence of the Text wrapping?

zivester commented 5 years ago

After doing some digging, I'm fairly confident this is a regression caused by #17 . I can't have both images and this working at the same time.

zivester commented 5 years ago

@dsherida I've created PR #23 that I believe should fix this issue and a couple others if you want to try it out.