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 prevent-pr-commit-link-loss feature #3085

Merged
merged 40 commits into from May 18, 2020
Merged

Add prevent-pr-commit-link-loss feature #3085

merged 40 commits into from May 18, 2020

Conversation

max-arias
Copy link
Contributor

@max-arias max-arias commented May 12, 2020

Closes: #2327


Note: I had to rename the URL to something other than the URL, otherwise Github replaces the URL anyways with the commit URL in the markdown 🤷‍♂️ .


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


@fregante
Copy link
Member

fregante commented May 13, 2020

Thank you for the PR! You can simplify this to a single delegate call with just event.delegateTarget.value.replace(…, …) inside. Only 3 lines inside init. No need to catch errors or use match

@max-arias
Copy link
Contributor Author

I'll try and implement it with delegate, never used it before 👍

@max-arias
Copy link
Contributor Author

Update @fregante , let me know what you think. Cheers.

Copy link
Member

@fregante fregante left a comment

Choose a reason for hiding this comment

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

Looks much shorter! Here’s a few more notes

@max-arias
Copy link
Contributor Author

@fregante Thanks a lot for the help. Pushed all suggestions and a few tests.

max-arias and others added 4 commits May 13, 2020 20:08
….tsx

Co-authored-by: Fregante <opensource@bfred.it>
Co-authored-by: Fregante <opensource@bfred.it>
@fregante
Copy link
Member

On further thought, this really cannot be done automatically. Proper replacement requires a Markdown parser and we can't import that.

If you can, I suggest reimplementing it as an actionable warning instead, so the user can make an informed decision.

Here's the code for this alert, but it needs more work:

select('.form-actions').prepend(
	<div class="flash flash-warn mb-2">
	  <AlertIcon /> Your PR Commit link may be <a href="https://github.com/sindresorhus/refined-github/issues/2327">misinterpreted by GitHub.</a>
	  <button type="button" class="btn btn-sm primary flash-action">Fix link</button>
	</div>
);

@fregante
Copy link
Member

I think it should also be removed once the regex no longer matches:

@max-arias
Copy link
Contributor Author

I think it should also be removed once the regex no longer matches:

Yup! I was just working on that. I'll push a fix soon.

@max-arias
Copy link
Contributor Author

Thanks for all the help @fregante

Is there anything pending?

@fregante fregante merged commit 2531c75 into refined-github:master May 18, 2020
@fregante
Copy link
Member

@max-arias this only works for comments. Can you make sure this also works when opening an issue or PR?

@yakov116
Copy link
Member

@max-arias I once started working on it and dont have time to continue.

@@ -27,7 +27,11 @@ function updateUI(event: delegate.Event<InputEvent, HTMLTextAreaElement>): void       
        const field = event.delegateTarget;

        if (prCommitRegex.test(field.value)) {
-               select('.form-actions', field.form!)!.prepend(getUI(field));
+               if (pageDetect.hasComments()) {
+                       select('.form-actions', field.form!)!.prepend(getUI(field));
+               } else {
+                       select('#new_issue .flex-items-center, #new_pull_request .flex-justify-end')!.before(<div className="form-actions"> 
{getUI(field)} </div>);
+               }
        } else {
                getUI(field).remove();
        }
@@ -44,7 +48,7 @@ features.add({   
        screenshot: 'https://user-images.githubusercontent.com/1402241/82131169-93fd5180-97d2-11ea-9695-97051c55091f.gif'
 }, {
        include: [
-               pageDetect.hasComments
+               pageDetect.hasRichTextEditor
        ],
        init
 });

@max-arias
Copy link
Contributor Author

Thanks @yakov116 . Little busy during the week, I'll see if I can put a few hours this weekend. Cheers.

@fregante
Copy link
Member

Clarification: it does work on the first comment of PRs and issues, but only on isIssue and isPR, not on the "new issue/pr" pages.

Also I noticed this 😅 (it's toggled at every keypress)

@fregante fregante mentioned this pull request Jun 3, 2020
@fregante
Copy link
Member

fregante commented Jun 5, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Fix PR commit links on submission (GitHub’s bug)
3 participants