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

[$4000] Long lines in code blocks should not wrap #7497

Closed
mvtglobally opened this issue Feb 1, 2022 · 63 comments
Closed

[$4000] Long lines in code blocks should not wrap #7497

mvtglobally opened this issue Feb 1, 2022 · 63 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@mvtglobally
Copy link

mvtglobally commented Feb 1, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Open app
  2. Send few long lines of code in chat
- name: Decrypt Botify GPG key
        run: cd .github/workflows && gpg --quiet --batch --yes --decrypt --passphrase="$LARGE_SECRET_PASSPHRASE" --output OSBotify-private-key.asc OSBotify-private-key.asc.gpg
        env:
          LARGE_SECRET_PASSPHRASE: ${{ secrets.LARGE_SECRET_PASSPHRASE }}

Expected Result:

Long lines in code blocks should not wrap

Actual Result:

Long lines in code blocks wrap

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • Desktop App

Version Number: 1.1.33-3
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Screen Shot 2022-02-01 at 1 10 00 AM

Expensify/Expensify Issue URL:
Issue reported by: @roryabraham
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1643397896087709

View all open jobs on GitHub

view this job

@MelvinBot
Copy link

Triggered auto assignment to @tylerkaraszewski (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@sobitneupane
Copy link
Contributor

Hello, Is this the expected result?
Screenshot from 2022-02-01 18-23-21

If yes, I will post a proposal.

@parasharrajat
Copy link
Member

@sobitneupane May be you can wait till issue is triaged.

@tylerkaraszewski tylerkaraszewski removed their assignment Feb 1, 2022
@tylerkaraszewski tylerkaraszewski added the External Added to denote the issue can be worked on by a contributor label Feb 1, 2022
@MelvinBot
Copy link

Triggered auto assignment to @jboniface (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@tylerkaraszewski
Copy link
Contributor

Seems fine, probably straightforward.

@jboniface
Copy link

@MelvinBot
Copy link

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (Exported)

@MelvinBot MelvinBot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 2, 2022
@MelvinBot
Copy link

Triggered auto assignment to @NikkiWines (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@rushatgabhane
Copy link
Member

rushatgabhane commented Feb 3, 2022

Hello, Is this the expected result?

@sobitneupane
Yes that's the expected result. Rory mentions on slack that it should behave like github.
Excited for your proposal!

@Viacheslav80
Copy link
Contributor

Viacheslav80 commented Feb 5, 2022

Hi!

Proposal

Create new PreRenderer.js for HTMLRenderers. Inside:

const PreRenderer = ({TDefaultRenderer, ...props}) => {
    const scrollRef = useRef(null);
    
    function onScroll(e) {
        scrollRef.current.scrollLeft += e.deltaX;
    }

    useEffect(() => {
        scrollRef.current.addEventListener('wheel', onScroll)
        
        return function cleanup() {
            scrollRef.current.removeEventListener('wheel', onScroll);
        };
    }, []);

    return (
        <ScrollView ref={scrollRef} horizontal style={props.style}>
            <TDefaultRenderer
                {...props}
                style={{ flexGrow: 1, flexShrink: 1, padding: 10 }}
            />
        </ScrollView>
    );
};

/src/components/HTMLEngineProvider/HTMLRenderers/index.js :

 14    img: ImageRenderer,
+15    pre: PreRenderer,

Screen.Recording.2022-02-05.at.22.42.51.mov

@rushatgabhane
Copy link
Member

@NikkiWines sorry, I can't review this one. I'm not well 🤒

@rushatgabhane rushatgabhane removed their assignment Feb 6, 2022
@parasharrajat
Copy link
Member

Taking over from @rushatgabhane as C+.

@sobitneupane I am interested to hear your proposal.

@jboniface
Copy link

bump -- any updates on this? it looks like we have a proposal without definitive response. cc @parasharrajat / @NikkiWines

@MelvinBot MelvinBot removed the Overdue label Feb 15, 2022
@parasharrajat
Copy link
Member

Oh, I see. I am not assigned to it. That's why. @jboniface Please assign me as C+. Thanks.

I like @Viacheslav80 's proposal. Please use the Class component. we don't use hooks.

cc: @NikkiWines

🎀 👀 🎀 C+ reviewed

@parasharrajat
Copy link
Member

Thank you @orkunkarakus for the detailed post.

Long Press context (No need in desktop and web versions)

Correct. But it should open on right-click.

Sounds like you have fixed all the issues. Do you mind sharing the technical details?

@orkunkarakus
Copy link
Contributor

Thank you @orkunkarakus for the detailed post.

@parasharrajat You're welcome.

Long Press context (No need in desktop and web versions)

Correct. But it should open on right-click.

Right click still works. Only long press is disabled on hoverable platforms.
Right click video.

web-right-click-expensify.mov

Sounds like you have fixed all the issues. Do you mind sharing the technical details?

Sure, The "react-native-render-html" library has a feature that allows us to render html tags with custom components.
( Explained in this page [Library documentation] https://meliorence.github.io/react-native-render-html/docs/guides/custom-renderers )

I wrote a custom render component for our code block tag using this feature. This custom render component includes the ScrollView component of the "react-native-gesture-handler" library. At the same time i used to "onStartShouldSetResponder" prop provided by the Gesture Responder System in the scrollview. This prop determine the responder is scrollview while scrolling, not the long press.

@parasharrajat
Copy link
Member

If the solution to the problem is ok for you, I can send my proposal to here.

Should I consider the above post as a proposal or do you want to share something else? It seems that you are still waiting on that.

@orkunkarakus
Copy link
Contributor

If the solution to the problem is ok for you, I can send my proposal to here.

Should I consider the above post as a proposal or do you want to share something else? It seems that you are still waiting on that.

if it is ok for you, you can consider it is a proposal.

@parasharrajat
Copy link
Member

Ok, considering that the @orkunkarakus mentioned that he fixed the scrolling issue without compromising other features, I am fine with @orkunkarakus 's proposal for the issue.

cc: @NikkiWines

🎀 👀 🎀 C+ reviewed

@NikkiWines
Copy link
Contributor

Yep, I agree - this proposal looks good. @jboniface could you hire @orkunkarakus for this role, please?

@melvin-bot
Copy link

melvin-bot bot commented Apr 26, 2022

📣 @orkunkarakus You have been assigned to this job by @jboniface!
Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 26, 2022
orkunkarakus added a commit to orkunkarakus/Expensify that referenced this issue Apr 26, 2022
@orkunkarakus
Copy link
Contributor

PR is created. @parasharrajat

cc: @NikkiWines

@melvin-bot melvin-bot bot added the Overdue label May 5, 2022
@jboniface jboniface added the Reviewing Has a PR in review label May 6, 2022
@melvin-bot melvin-bot bot removed the Overdue label May 6, 2022
NikkiWines added a commit that referenced this issue May 9, 2022
Long lines in code blocks should not wrap issue (#7497) is fixed
@parasharrajat
Copy link
Member

parasharrajat commented May 16, 2022

@orkunkarakus I just noticed one thing that I forgot to mention during the PR review.
when we are scrolling the chat list via wheel and mouse comes over the code block, the scroll is blocked.

As the regression period is still active, could you please submit a PR to fix that? It should be a small fix.

The expected behavior would be when the user is scrolling horizontally, only then code block should scroll and scrolling chat should not be blocked via code block while scrolling vertically.

Steps:

  1. Open any chat.
  2. Send a message would code block as per the issue or use an old message in the history.
  3. Send a few more messages.
  4. Use a touchpad or mouse wheel to scroll the chat message list.
  5. As soon as the cursor comes over the code block, the scroll is blocked.

@orkunkarakus
Copy link
Contributor

orkunkarakus commented May 16, 2022

@orkunkarakus I just noticed one thing that I forgot to mention during the PR review. when we are scrolling the chat list via wheel and mouse comes over the code block, the scroll is blocked.

As the regression period is still active, could you please submit a PR to fix that? It should be a small fix.

The expected behavior would be when the user is scrolling horizontally, only then code block should scroll and scrolling chat should not be blocked via code block while scrolling vertically.

Steps:

  1. Open any chat.
  2. Send a message would code block as per the issue or use an old message in the history.
  3. Send a few more messages.
  4. Use a touchpad or mouse wheel to scroll the chat message list.
  5. As soon as the cursor comes over the code block, the scroll is blocked.

@parasharrajat Since the update is currently in staging, should I create a new branch from staging or through the old merged branch (orkunkarakus:fix/7497-LongLinesInCode) ?
or is it enough for you to update the old branch and open pr again?

@parasharrajat
Copy link
Member

parasharrajat commented May 16, 2022

I think you can start from the main branch otherwise, just start from your old PR and merge the main into it. Thanks for considering the request.

@orkunkarakus
Copy link
Contributor

orkunkarakus commented May 16, 2022

I think you can start from the main branch otherwise, just start from your old PR and merge the main into it. Thanks for considering the request.

@parasharrajat You're welcome. I opened the PR #9026

NikkiWines added a commit that referenced this issue May 18, 2022
Message list not vertically scrolling while mouse over horizontal scrollable code blocks (#7497) is fixed [Duplicate for vertical scroll hotfix]
@kadiealexander
Copy link
Contributor

Issued payment to @orkunkarakus after email bump.

@parasharrajat
Copy link
Member

This is ready for payout.

@mallenexpensify
Copy link
Contributor

mallenexpensify commented May 31, 2022

Paid @parasharrajat $4k for C+ on a different job than @orkunkarakus (in case anyone is auditing later on).
Closing, thanks for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests