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 show more text component #3736

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add show more text component #3736

wants to merge 2 commits into from

Conversation

jmpas
Copy link
Member

@jmpas jmpas commented Sep 25, 2023

Simulator.Screen.Recording.-.iPhone.14.Plus.-.2023-09-25.at.13.44.51.mp4

@@ -0,0 +1,33 @@
import React from 'react';

import ReadMore from '@fawazahmed/react-native-read-more';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain the benefits of this lib? Couldn't we just do this with a character count and simple state in this component? The lib is a bit sloppy built (e.g. hard to find the source code) so I'm a bit hesitant at adding it.

Copy link
Member Author

@jmpas jmpas Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aha, I approached it similarly (mindset wise) using

onTextLayout={({ nativeEvent: { lines } }) =>
    setShowMoreButton(lines.length > numberOfLines)
    ...

But it didn't work well at all, would be interesting trying with character count as you mentioned
(also couldn't add the show more button by the end of the last line, but that's not a required thing)

Copy link
Member Author

@jmpas jmpas Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like counting characters wouldn't work without a custom parser, because we would need to count \n\n` differently, since it fills more space, as the following:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the example above I'm using numberOfLines={5} on the text to limit

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expo seems to have a lib for this as well https://github.com/expo/react-native-read-more-text

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall I copy their approach and do a component in our codebase? wdyt? @gewfy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing some research here! I believe we did something similar, height based, in the old. Let me check :)

Copy link
Member

@gewfy gewfy Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah exactly, in the old ap we did what you proposed ⬆️

                onTextLayout={({ nativeEvent: { lines } }) => {
                  if (lines.length > NUMBER_OF_LINES && isNil(truncated)) {
                    setTruncated(true);
                  }
                }}

And then just absolute position a Show more at the right bottom of that component, or bellow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this seems more complex than it looks on the surface I leave it up to you. Fine to go with the lib you chose too :)

@@ -259,7 +259,7 @@ const CompletedSessionModal = () => {
<>
<Spacer16 />
<Gutters>
<Markdown>{exercise?.description}</Markdown>
<ShowMoreText>{exercise?.description}</ShowMoreText>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny thing. Aren't we loosing the possibility to use markdown with this? Wrap it in <ShowMoreText>?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Markdown doesn't seem to support numberOfLines or similar functionality, does it? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah you mean using the library, I need to try that out

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