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.

Sign up for GitHub

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

Jump to bottom

small-user-avatars - Don't mistakenly add to commit authors #7139

Merged
merged 2 commits into from
Dec 6, 2023
Merged

small-user-avatars - Don't mistakenly add to commit authors #7139

merged 2 commits into from
Dec 6, 2023

Conversation

dgw
Copy link
Contributor

@dgw dgw commented Dec 4, 2023

Fix #7137 based on #7137 (comment)

Test URLs

sopel-irc/sopel@120477f

Screenshot

Before:
image

After:
image

Verified that small-user-avatars are still added on regular mentions in comments, e.g. sopel-irc/sopel#2533 (comment)

@dgw dgw mentioned this pull request Dec 4, 2023
Closed
fregante
fregante approved these changes Dec 4, 2023
View reviewed changes
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! Can you also call assertNodeContent on line 23-ish? It should test the regex /^@/

@dgw dgw force-pushed the small-user-avatars/no-commit-author branch from ef8c1e8 to 7a7c2ed Compare December 4, 2023 23:04
@dgw
small-user-avatars - fix: ignore fake commit-author mentions
50e44a7 
`user-mention` link without a leading `@` symbol will log an error via
`assertNodeContent` helper.
@dgw dgw force-pushed the small-user-avatars/no-commit-author branch from 7a7c2ed to 50e44a7 Compare December 4, 2023 23:18
@dgw
Copy link
Contributor Author

dgw commented Dec 4, 2023 •
edited
Loading

Here we go. Had to learn about the difference between regular and {} imports (I am clearly not a JS dev!), then temporarily reverted the actual fix in my working copy to make sure the assertNodeContent call functioned:

image

Amended for you, with no linter error this time 🙂

@dgw dgw marked this pull request as ready for review December 4, 2023 23:20
@fregante fregante changed the title small-user-avatars - fix: ignore fake commit-author mentions small-user-avatars - Don't mistakenly add to commit authors Dec 6, 2023
@fregante fregante added the bug label Dec 6, 2023
@fregante fregante enabled auto-merge (squash) December 6, 2023 11:57
@fregante fregante merged commit 38e6dcf into refined-github:main Dec 6, 2023
10 checks passed
@fregante
Copy link
Member

fregante commented Dec 16, 2023 •
edited
Loading

I think this broke the entire feature. See on https://github.com/refined-github/refined-github/releases

Screenshot

The assertion error is confusing, but I think it's failing because the assertion expects a text node ( like link.firstChild), not an element (like link)

@dgw dgw deleted the small-user-avatars/no-commit-author branch December 16, 2023 20:21
@dgw
Copy link
Contributor Author

dgw commented Dec 16, 2023

Figured out what I did wrong! PR coming shortly with a fix.

@dgw dgw mentioned this pull request Dec 16, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fregante fregante fregante approved these changes

Assignees
No one assigned
Labels
bug
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

small-user-avatars showing wrong avatars on commit history/details
2 participants
@dgw @fregante