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 draggable false to button markup and validation #224

Merged
merged 1 commit into from
Nov 19, 2021

Conversation

owenatgov
Copy link
Contributor

What

Adds draggable="false" to the govspeak button extension output.

Why

This is in response to a change to button markup in the components gem (alphagov/govuk_publishing_components#2448) which wasn't reflected in govspeak. This caused the govspeak validator to assume all buttons failed as the in-app markup was including draggable="false" whilst sanitised govspeak wasn't.

Zendesk ticket which prompted this

No visual changes

@owenatgov owenatgov force-pushed the draggable-false-on-buttons branch from b063d76 to 390ba29 Compare November 19, 2021 12:35
@owenatgov owenatgov marked this pull request as ready for review November 19, 2021 12:35
Copy link

@karlbaker02 karlbaker02 left a comment

Choose a reason for hiding this comment

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

Looks good, a few small comments and questions

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -224,7 +224,7 @@ def insert_strong_inside_p(body, parser = Govspeak::Document)
text = text.strip
href = href.strip

%(\n<a role="button" class="#{button_classes}" href="#{href}" #{data_attribute}>#{text}</a>\n)
%(\n<a role="button" draggable="false" class="#{button_classes}" href="#{href}" #{data_attribute}>#{text}</a>\n)

Choose a reason for hiding this comment

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

We still need draggable="false" here right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After a very quick test, we indeed do.

@owenatgov owenatgov force-pushed the draggable-false-on-buttons branch from 390ba29 to ad62d2c Compare November 19, 2021 12:46
This is in response to a change to button markup in the components gem which wasn't reflected in govspeak. This caused the govspeak validator to assume all buttons failed as the in-app markup was including `draggable="false"` whilst sanitised govspeak wasn't.
@owenatgov owenatgov force-pushed the draggable-false-on-buttons branch from ad62d2c to 3198495 Compare November 19, 2021 12:48
Copy link

@karlbaker02 karlbaker02 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks Owen! 👍

@owenatgov owenatgov merged commit ad24b9a into main Nov 19, 2021
@owenatgov owenatgov deleted the draggable-false-on-buttons branch November 19, 2021 13:37
@owenatgov owenatgov mentioned this pull request Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants