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

[FIX]: Fix PR expand Feature #142

Merged
merged 3 commits into from
Mar 31, 2024

Conversation

VipinDevelops
Copy link
Contributor

@VipinDevelops VipinDevelops commented Mar 6, 2024

Issue(s)

Acceptance Criteria Fulfillment

  • Fix the issue of loading multiple buttons after obtaining the pull link by using IPreMessageSentExtend.
  • Add a proper check for the number of PRs in a message and render a different button to handle each PR, with a maximum of 3 links.
  • Add the "Approve" button in the PR details modal to quickly approve changes.

Proposed Changes (including videos or screenshots)

Screencast.from.2024-03-06.22-54-35.webm

Comment on lines +19 to +21
while ((prLinkMatches = githubPRLinkRegex.exec(text)) !== null) {
matches.push(prLinkMatches);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This while loop is not making sense

Copy link
Contributor Author

@VipinDevelops VipinDevelops Mar 11, 2024

Choose a reason for hiding this comment

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

Hey @samad-yar-khan Actually We are using this loop to get all the possible PR links from the message.

This while loop iterates through the matches of the regular expression in the message text. It uses exec() to find the next match in the text. As long as there is a match (!== null), it adds the match to the matches array.

and now that we have an array of all possible PR links we can check if its more then 3 let's not show a button or else add the button for Each PR.

@samad-yar-khan
Copy link
Collaborator

samad-yar-khan commented Mar 10, 2024

@VipinDevelops Great improvement over the previous implementation, user experience feels cleaner !

@VipinDevelops
Copy link
Contributor Author

@VipinDevelops Great improvement over the previous implementation, user experience feels cleaner !

I am glad you like this approach it took me some time to figure out the best alternative for this.

Comment on lines 84 to +88
await handleGitHubCodeSegmentLink(message, read, http, message.sender, message.room, extend);
}

if (await hasGithubPRLink(message)) {
await handleGithubPRLinks(message, read, http, message.sender, message.room, extend);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@VipinDevelops do you think adding more of these checks in checkPreMessageSentExtend will hinder performance ?

Copy link
Collaborator

@samad-yar-khan samad-yar-khan left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks @VipinDevelops

@samad-yar-khan samad-yar-khan merged commit 1f754ad into RocketChat:main Mar 31, 2024
1 check passed
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