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 isNewDiscussion and add it to hasRichTextEditor #177

Merged

Conversation

kang8
Copy link
Contributor

@kang8 kang8 commented Jul 13, 2023

@kang8 kang8 force-pushed the add-new-discussions-url-to-is-discussion branch from 0ba4476 to a31e4a8 Compare July 13, 2023 11:05
index.ts Outdated Show resolved Hide resolved
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.

Thank you! 💚

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.

Actually this is not right. The “new discussion” page is not a “discussion”, there’s no discussion there.

#175 mentions hasRichTextEditor. I think this should be moved to isNewDiscussion and then isNewDiscussion should be added to hasRichTextEditor

@kang8 kang8 force-pushed the add-new-discussions-url-to-is-discussion branch from f3405fc to 5868486 Compare July 14, 2023 01:18
@kang8 kang8 changed the title Add new discussions url to isDiscussion() Add isNewDiscussion and add it to hasRichTextEditor Jul 14, 2023
index.ts Outdated Show resolved Hide resolved
index.ts Outdated
@@ -213,6 +214,13 @@ addTests('isDiscussion', [
'https://github.com/orgs/community/discussions/11202',
]);

export const isNewDiscussion = (url: URL | HTMLAnchorElement | Location = location): boolean => (getRepo(url)?.path === 'discussions/new' || getOrg(url)?.path === 'discussions/new')
&& new URLSearchParams(url.search).get('category') !== null;
Copy link
Member

@fregante fregante Jul 23, 2023

Choose a reason for hiding this comment

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

Why? If you remove the category, it's redirected to https://github.com/withastro/roadmap/discussions/new/choose

Also sindresorhus/meta#7. I'm not sure why this lint rule isn't complaining https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/no-null.md

Suggested change
&& new URLSearchParams(url.search).get('category') !== null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you remove the category, it's redirected to withastro/roadmap/discussions/new/choose

Yes, But is withastro/roadmap/discussions/new/choose a new discussions? Because I'm referring to isNewIssue(), which does not match issue/new/choose, see Test CI for more details.

Also sindresorhus/meta#7. I'm not sure why this lint rule isn't complaining sindresorhus/eslint-plugin-unicorn@main/docs/rules/no-null.md

The no-null has an option: checkStrictEquality, which is set to false by default. It must be manually enabled:

  	"xo": {
		"overrides": [
			"rules": {
			...
+			"unicorn/no-null" : ["error", { "checkStrictEquality": false }],
			}
		]
	}

Copy link
Member

Choose a reason for hiding this comment

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

is withastro/roadmap/discussions/new/choose a new discussions

It's not and it's not matched by getRepo(url)?.path === 'discussions/new' || getOrg(url)?.path === 'discussions/new' already. Redirects are not taken into considerations because github-url-detection is meant to be used on real/final URLs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏽 I have already removed the category check.

@kang8 kang8 force-pushed the add-new-discussions-url-to-is-discussion branch from 49df56f to 1114e0a Compare July 25, 2023 14:44
@fregante fregante merged commit 2b236f1 into refined-github:main Jul 31, 2023
@fregante fregante added the enhancement New feature or request label Jul 31, 2023
@fregante
Copy link
Member

Thank you!

@kang8 kang8 deleted the add-new-discussions-url-to-is-discussion branch July 31, 2023 09:43
@fregante fregante linked an issue Jul 31, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

New discussions are not included in hasRichTextEditor
3 participants