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

Disable Test Delivery and Replay webhook buttons when webhook is inactive #27211

Merged
merged 3 commits into from
Sep 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -2133,12 +2133,14 @@ settings.webhook_deletion_desc = Removing a webhook deletes its settings and del
settings.webhook_deletion_success = The webhook has been removed.
settings.webhook.test_delivery = Test Delivery
settings.webhook.test_delivery_desc = Test this webhook with a fake event.
settings.webhook.test_delivery_desc_disabled = To test this webhook with a fake event, activate it.
settings.webhook.request = Request
settings.webhook.response = Response
settings.webhook.headers = Headers
settings.webhook.payload = Content
settings.webhook.body = Body
settings.webhook.replay.description = Replay this webhook.
settings.webhook.replay.description_disabled = To replay this webhook, activate it.
settings.webhook.delivery.success = An event has been added to the delivery queue. It may take few seconds before it shows up in the delivery history.
settings.githooks_desc = "Git Hooks are powered by Git itself. You can edit hook files below to set up custom operations."
settings.githook_edit_desc = If the hook is inactive, sample content will be presented. Leaving content to an empty value will disable this hook.
Expand Down
10 changes: 8 additions & 2 deletions templates/repo/settings/webhook/history.tmpl
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
{{$isNew:=or .PageIsSettingsHooksNew .PageIsAdminDefaultHooksNew .PageIsAdminSystemHooksNew}}
{{if .PageIsSettingsHooksEdit}}
<h4 class="ui top attached header">
{{.locale.Tr "repo.settings.recent_deliveries"}}
{{if .Permission.IsAdmin}}
<div class="ui right">
<button class="ui teal tiny button" id="test-delivery" data-tooltip-content="{{.locale.Tr "repo.settings.webhook.test_delivery_desc"}}" data-link="{{.Link}}/test" data-redirect="{{.Link}}">{{.locale.Tr "repo.settings.webhook.test_delivery"}}</button>
<!-- the button is wrapped with a span because the tooltip doesn't show on hover if we put data-tooltip-content directly on the button -->
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know why the tooltip doesn't show on a disabled button so I wrapped it in a span

Copy link
Member

Choose a reason for hiding this comment

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

I think this is per browser design where disabled elements do not emit mouse events and such:

https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/disabled#overview
atomiks/tippyjs#286

Often browsers grey out such controls and it won't receive any browsing events, like mouse clicks or focus-related ones.

Copy link
Member

@silverwind silverwind Sep 24, 2023

Choose a reason for hiding this comment

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

Thought I see we don't actually set disabled attribute, only disabled class, so it should still work unless the CSS sets pointer-events: none on it.

Copy link
Member

Choose a reason for hiding this comment

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

Actually yes, I see .ui.disabled.button sets pointer-events: none so that is likely why tippy does not work. I guess a wrapper is a ok-ish solution.

Copy link
Member

Choose a reason for hiding this comment

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

That's even the official solution, apparently.

Copy link
Member

@silverwind silverwind Sep 24, 2023

Choose a reason for hiding this comment

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

We do have a little bit more control because our CSS sets the pointer-events, but I can't recommend enabling pointer-events because that would make the button clickable and it would need additional JS to ensure it's unclickable.

<span data-tooltip-content="{{if or $isNew .Webhook.IsActive}}{{.locale.Tr "repo.settings.webhook.test_delivery_desc"}}{{else}}{{.locale.Tr "repo.settings.webhook.test_delivery_desc_disabled"}}{{end}}">
<button class="ui teal tiny button{{if not (or $isNew .Webhook.IsActive)}} disabled{{end}}" id="test-delivery" data-link="{{.Link}}/test" data-redirect="{{.Link}}">{{.locale.Tr "repo.settings.webhook.test_delivery"}}</button>
</span>
</div>
{{end}}
</h4>
Expand Down Expand Up @@ -43,7 +47,9 @@
<div class="right menu">
<form class="item" action="{{$.Link}}/replay/{{.UUID}}" method="post">
{{$.CsrfTokenHtml}}
<button class="ui tiny button" data-tooltip-content="{{$.locale.Tr "repo.settings.webhook.replay.description"}}">{{svg "octicon-sync"}}</button>
<span data-tooltip-content="{{if $.Webhook.IsActive}}{{$.locale.Tr "repo.settings.webhook.replay.description"}}{{else}}{{$.locale.Tr "repo.settings.webhook.replay.description_disabled"}}{{end}}">
<button class="ui tiny button{{if not $.Webhook.IsActive}} disabled{{end}}">{{svg "octicon-sync"}}</button>
</span>
</form>
</div>
{{end}}
Expand Down