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

Restore support for unmarked bots in dim-bots #5946

Merged
merged 4 commits into from
Sep 8, 2022

Conversation

gaetanmaisse
Copy link
Contributor

Issue

Closes #5945
Follow the work done in #5512

Test URLs

Check the PRs opened in: https://github.com/gravitee-io/gravitee-policy-oauth2/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc

If there is no PRs opened by Renovate or Snyk by the time this PR is reviewed please refer to the screenshot below ⬇️

Screenshot

Before
image

After

image

prSelectors.push(
...botNames.map(bot => `.opened-by [title*="pull requests opened by ${bot}"]`),
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 kept it because it was there but I'm not sure where it is used (or not anymore?)

Copy link
Member

Choose a reason for hiding this comment

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

I presume that the copy was just updated and it can now be replaced instead of extended

@gaetanmaisse gaetanmaisse marked this pull request as ready for review September 5, 2022 19:19
@fregante fregante changed the title Fix dim of PRs opened by snyk-bot Restore support for unmarked bots in dim-bots Sep 8, 2022
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.

Thanks for the PR!

On a (semi-un-)related note:

As I found myself dealing with bots more often, I've found our UI a bit clunky to work with.

What are your thoughts on this alternative behavior?

  • dim and collapse them (same as now)
  • un-dim them on hover (50% of what we do now)
  • add "Expand bots" text on hover (even just via .bot:after {content: "expand"} 🆕
  • expand and undim all bots when clicking on any non-link on a bot line (which would include this fake "expand" link) 🆕

This would keep the bots dimmed until you start interacting with them, at which point the feature is effectively disabled until a reload.

We only need to ensure to update the scroll position after the click (like we do with toggle-all-with-alt)

@fregante fregante mentioned this pull request Sep 8, 2022
3 tasks
@gaetanmaisse
Copy link
Contributor Author

About the new behavior, you propose let me double-check what I'm really doing when I interact with bot's PRs. In any case I would be more than happy to help and contribute :)

Co-authored-by: Gaëtan Maisse <gaetanmaisse@gmail.com>
@fregante fregante added the bug label Sep 8, 2022
@fregante fregante merged commit 4a9f445 into refined-github:main Sep 8, 2022
@fregante
Copy link
Member

fregante commented Sep 8, 2022

About the new behavior, you propose let me double-check what I'm really doing when I interact with bot's PRs. In any case I would be more than happy to help and contribute :)

If you'd like to try, the easiest way would be something like:

function handler (event) {
  if (event.delegateTarget.closest('a, button, summary')) {
		return
	}
	document.body.classList.add('rgh-no-mo-dim-bot')
}
delegate(document, 'click', '.rgh-dim-bot', handler, {signal})

We don't need to actually add the "Expand bots" label at this point. I tried it and it's a little awkward to position with CSS only. Maybe I'd like a robot icon, but still difficult to make it follow the text color while living in the CSS file as well.

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

Successfully merging this pull request may close these issues.

Dim PRs opened by snyk-bot
3 participants