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

feat: including "replying to" in copy text (resolve #172) #188

Closed
wants to merge 4 commits into from
Closed

feat: including "replying to" in copy text (resolve #172) #188

wants to merge 4 commits into from

Conversation

kimlimjustin
Copy link

Please make sure to check the following tasks before opening and submitting a PR

  • I understand and have followed the contribution guide
  • I have tested my changes locally and they are working as intended
  • These changes do not have any notable side effects on other Revolt projects

Tricking copying "replying to" text into the keyboard by adding an invisible element, as suggested at #172. Hope this changes look good :)

@github-actions
Copy link

github-actions bot commented Apr 23, 2023

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@kimlimjustin
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@kimlimjustin
Copy link
Author

recheck

@Rexogamer Rexogamer changed the title Feat: including "replying to" in copy text (resolve #172) feat: including "replying to" in copy text (resolve #172) Apr 24, 2023
@insertish
Copy link
Member

Would it not be possible to include hidden text directly in the MessageReply component rather than refactoring the Message component? (and duplicating some code)

@kimlimjustin
Copy link
Author

Would it not be possible to include hidden text directly in the MessageReply component rather than refactoring the Message component? (and duplicating some code)

Sorry for the late response... I've pushed a commit, could you please recheck it?

Copy link
Member

@insertish insertish left a comment

Choose a reason for hiding this comment

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

Sorry currently doing exams but, there is still quite a large diff on this PR.

There is at least one issue reading this that could cause a crash but beyond that I would probably refactor this further into just one file, in theory you should be able to just define the "ReplyHelperComponent" styled span component in MessageReply itself and then use it as appropriate.

</NonBreakingText>
<Show when={props.message!.attachments}>
<>
<MessageReplyCopyHelper message={props.message!} />
Copy link
Member

Choose a reason for hiding this comment

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

If props.message is not yet defined then this would crash out.

</NonBreakingText>
</Show>
<Show when={props.message!.content}>
<OverflowingText>
Copy link
Member

Choose a reason for hiding this comment

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

Invisible text could be placed above this with user-select enabled.
(the reply span styled component would also be defined in this file)

@insertish insertish marked this pull request as draft May 4, 2023 19:01
@insertish
Copy link
Member

PR went stale, feedback not responded to, closing out for now

@insertish insertish closed this Dec 1, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants