matrix-react-sdk icon indicating copy to clipboard operation
matrix-react-sdk copied to clipboard

In chat export, make sender name differentiable using `< >`

Open rameshsinghppc opened this issue 2 years ago • 17 comments

Fixes https://github.com/vector-im/element-web/issues/23837

Checklist

  • [x] Tests written for new code (and old code if feasible)
  • [x] Linter and other CI checks pass
  • [x] Sign-off given on the changes (see CONTRIBUTING.md)

Signed-off-by: Ramesh Singh @rameshsinghppc

Type: [enhancement/defect/task]


Here's what your changelog entry will look like:

🐛 Bug Fixes

  • In chat export, make sender name differentiable using < > (#10748). Fixes vector-im/element-web#23837. Contributed by @rameshsinghppc.

rameshsinghppc avatar Apr 29 '23 12:04 rameshsinghppc

This appears to be much the same as https://github.com/matrix-org/matrix-react-sdk/pull/10013, and as such has the same problems:

This code is used throughout the whole product and not only in the context of chat exports. You will also need to write tests for this PR to be merged as per then contributing guidelines https://github.com/vector-im/element-web/blob/develop/CONTRIBUTING.md

Please read CONTRIBUTING.md which gives instructions on how to make a good pull request. Looking at the format of other pull requests on the repository should help too.

richvdh avatar May 03 '23 11:05 richvdh

Fixes vector-im/element-web#23837

I have sent PR(https://github.com/matrix-org/matrix-js-sdk/pull/3345#issue-1698769126), please review it

rameshsinghppc avatar May 06 '23 20:05 rameshsinghppc

Both this and the other new PR are currently not in a state that can be reviewed or merged.

If you genuinely want to contribute this, I'd suggest adding another parameter to the functions in TextForEvent and using that to control whether the sender name will be wrapped in brackets. Take a look at the parameter that's used for controlling whether hidden events are included as an example.

justjanne avatar May 08 '23 13:05 justjanne

Screenshot from 2023-05-23 23-00-44

rameshsinghppc avatar May 23 '23 17:05 rameshsinghppc

@rameshsinghppc are you still keen to work on this? If you need pointers on how to fix up the CI failures then let us know.

richvdh avatar Jun 03 '23 14:06 richvdh

@rameshsinghppc are you still keen to work on this? If you need pointers on how to fix up the CI failures then let us know.

@richvdh Yes, please help me to fix these failures.

rameshsinghppc avatar Jun 04 '23 06:06 rameshsinghppc

@richvdh Yes, please help me to fix these failures.

Ok, what have you tried so far and which parts are confusing you?

richvdh avatar Jun 05 '23 08:06 richvdh

@richvdh Yes, please help me to fix these failures.

Ok, what have you tried so far and which parts are confusing you? I don't know how to fix the eslint problem and for typescript syntax check some function are not available during pull request checks but on my local machine it's working fine. Screenshot from 2023-05-28 15-09-44

rameshsinghppc avatar Jun 08 '23 16:06 rameshsinghppc

Right; a lot of the problem here is that it is building against your fork of matrix-js-sdk (https://github.com/rameshsinghppc/matrix-js-sdk) which is outdated. (We have some magical branch matching.) You can see this happening at https://github.com/matrix-org/matrix-react-sdk/actions/runs/5159164960/jobs/9322532146#step:4:48.

Suggest, as a first step, you pull the latest develop into your fork of matrix-js-sdk. Ping me once you've done so and I'll re-run CI here.

richvdh avatar Jun 09 '23 10:06 richvdh

Right; a lot of the problem here is that it is building against your fork of matrix-js-sdk (https://github.com/rameshsinghppc/matrix-js-sdk) which is outdated. (We have some magical branch matching.) You can see this happening at https://github.com/matrix-org/matrix-react-sdk/actions/runs/5159164960/jobs/9322532146#step:4:48.

Suggest, as a first step, you pull the latest develop into your fork of matrix-js-sdk. Ping me once you've done so and I'll re-run CI here.

To be more accurate here: it's actually building against the branch named rameshsingh in your fork of matrix-js-sdk. (Our magic branch-matching will use a js-sdk branch with the same name as your react-sdk branch).

So you need to update that branch (or just delete it).

richvdh avatar Jun 09 '23 11:06 richvdh

Right; a lot of the problem here is that it is building against your fork of matrix-js-sdk (https://github.com/rameshsinghppc/matrix-js-sdk) which is outdated. (We have some magical branch matching.) You can see this happening at https://github.com/matrix-org/matrix-react-sdk/actions/runs/5159164960/jobs/9322532146#step:4:48. Suggest, as a first step, you pull the latest develop into your fork of matrix-js-sdk. Ping me once you've done so and I'll re-run CI here.

To be more accurate here: it's actually building against the branch named rameshsingh in your fork of matrix-js-sdk. (Our magic branch-matching will use a js-sdk branch with the same name as your react-sdk branch).

So you need to update that branch (or just delete it).

@richvdh Thank you for your help. I just deleted that repository.

rameshsinghppc avatar Jun 09 '23 14:06 rameshsinghppc

you don't need to paste a screenshot - I can see the errors in CI.

Look at the line of code it is complaining about:

else return textForEvent(mxEv, this.room.client,true) + mediaText;

What is the return type of textForEvent? (Hint: look at the typescript definitions and consider which of the two prototypes your code is calling). Is it definitely a valid type to use in a string concatenation?

richvdh avatar Jun 09 '23 15:06 richvdh

@rameshsinghppc once you have made some changes you think will fix the CI please say so in a comment.

richvdh avatar Jun 12 '23 17:06 richvdh

@rameshsinghppc once you have made some changes you think will fix the CI please say so in a comment.

@richvdh No sir, It's contributor's responsibility to resolve all the issues. This is my first open source contribution, so I don't know the steps for the open source contribution. I want to do open source contribution and learn through that. One thing also I know that reviewer are very busy they don't have time for bad pull request but I am not getting any other ways through that I can learn open source contribution. If you think my pull request is causing some problems for the maintainers then I am really sorry for that, If you want then I can close this PR.

rameshsinghppc avatar Jun 12 '23 18:06 rameshsinghppc

@richvdh Few days back I made some changes in the code, please re- run the CI.

rameshsinghppc avatar Jun 12 '23 18:06 rameshsinghppc

@richvdh Today I have updated the code with eslint and test cases passed. Please re-run the CI. Screenshot from 2023-06-13 22-43-49 Screenshot from 2023-06-13 22-43-30

rameshsinghppc avatar Jun 13 '23 17:06 rameshsinghppc

@rameshsinghppc there's no need to merge develop every day.

richvdh avatar Jun 20 '23 15:06 richvdh

Thanks for your contribution. I'm going to close this for now as the changes requested haven't been made. If you're interested in continuing to work on this please let us know and we can reopen the pull request. Thanks!

richvdh avatar May 17 '24 11:05 richvdh