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 hasFooEditor #105

Merged
merged 9 commits into from
Nov 24, 2021
Merged

Add hasFooEditor #105

merged 9 commits into from
Nov 24, 2021

Conversation

kidonng
Copy link
Member

@kidonng kidonng commented Nov 22, 2021

Reason: both kind of pages are almost identical ("editing something").

While both are frequently used together, sometimes isNewFoo is easily left out since it's a special case that doesn't matter most of the time.

To replicate the old behavior of isEditingFoo, use isEditingFoo() && !isNewFoo().

We need some utils to check for these pages at the same time.

Changes:

  • Add hasFileEditor: isEditingFile, isNewFile and isDeletingFile
  • Add hasReleaseEditor: isEditingRelease and isNewRelease
  • Add hasWikiPageEditor: isEditingWikiPage and isNewWikiPage
  • hasRichTextEditor checks for hasReleaseEditor instead of isEditingRelease

@kidonng
Copy link
Member Author

kidonng commented Nov 22, 2021

@fregante Just curious, is there anything preventing this repo from migrating to @refined-github? Or do you simply prefer to keep it as-is?

@fregante
Copy link
Member

fregante commented Nov 23, 2021

Good idea, I think it'd be better to add isFooEditor and leave those alone.

I chose not to migrate it for now

@kidonng
Copy link
Member Author

kidonng commented Nov 23, 2021

Good idea, I think it'd be better to add isFooEditor and leave those alone.

Sounds good, breaking changes matter. I do consider the name a little bit weird though.

Regardless of how this is going, hasRichTextEditor needs to be updated and I'm going to make a PR in RGH to go through every instances of these includes.

@fregante
Copy link
Member

fregante commented Nov 23, 2021

Sounds good, breaking changes matter. I do consider the name a little bit weird though.

"Weird" in the sense that there's no "file editor" page. Is "hasFileEditor" better?

@kidonng kidonng changed the title Include isNewFoo in isEditingFoo Add hasFooEditor Nov 24, 2021
index.ts Outdated
@@ -298,16 +298,37 @@ collect.set('isEditingFile', [
'https://github.com/sindresorhus/refined-github/edit/ghe-injection/source/background.ts',
]);

export const hasFileEditor = (url: URL | HTMLAnchorElement | Location = location): boolean => isEditingFile(url) || isNewFile(url) || isDeletingFile(url);
collect.set('hasFileEditor', [
Copy link
Member

Choose a reason for hiding this comment

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

You can also mark them as combinedTestOnly to avoid having to list the URLs. But it's fine either way

@fregante
Copy link
Member

You can merge this PR, I added you as a collab. You can release a new minor version: https://github.com/fregante/github-url-detection/actions/workflows/npm-publish.yml

Screen Shot

patch is for fixes
minor is for new detections
major is rare, breaking changes

@fregante
Copy link
Member

@fregante Just curious, is there anything preventing this repo from migrating to @refined-github? Or do you simply prefer to keep it as-is?

I will try to move this repo to the organization, but I have to set up redirects for the github.io domain, change the meta, and then ensure that the secrets still work.

You can help by opening a PR to make the meta changes (fregante/github-url-detection -> refined-github/github-url-detection)

Ideally we'd also have the same npm auto-publishing setup on the other repository, so PR welcome there too to copy the workflow.

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

Successfully merging this pull request may close these issues.

None yet

2 participants