#43929 closed defect (bug) (fixed)
Privacy pages: buttons should be buttons and other coding standards
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 4.9.6 | Priority: | normal |
Severity: | normal | Version: | 5.1 |
Component: | Privacy | Keywords: | gdpr has-patch needs-testing |
Focuses: | coding-standards | Cc: |
Description
Starting with [42986] and following commits, a few buttons were added in the Privacy pages, to allow users export and remove their personal data. These buttons are actually links with a href="#"
attribute (9 occurrences).
As per the accessibility standards (https://make.wordpress.org/core/handbook/best-practices/coding-standards/accessibility-coding-standards/#semantics-for-controls) these links should be real buttons:
<button type="button" ...
Worth noting the type="button"
attribute allows to not use event.preventDefault()
as there's no default action to prevent.
===
CSS selectors:
As per the CSS coding standards (https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#selectors) words should be separated with hyphens, however many selectors in these pages use underscores:
https://make.wordpress.org/core/handbook/best-practices/coding-standards/css/#selectors
===
JavaSCript variable declarations with var
As per the JavaScript coding standards (https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#declaring-variables-with-var), multiple var
var
var
declarations should be avoided:
Each function should begin with a single comma-delimited var statement that declares any local variables necessary.
I know this is going to change, but the current standards are very clear. I'd say either update the coding standards, or meet them :)
Attachments (6)
Change History (25)
This ticket was mentioned in Slack in #core by sergey. View the logs.
7 years ago
#5
@
7 years ago
Did a bunch of testing on this. Everything seems to be working for me. 43929.3.diff converts a few more <a href="#"
occurrences to buttons.
#6
@
7 years ago
- Keywords needs-testing removed
Hey,
I was typing something about type="button"
missing parameter, but it seems fine with 43929.3.diff
now. I tested the patch on my side and I think it is okay to land in 4.9.6. Thanks.
#7
@
7 years ago
…but I guess you don't need no event.preventDefault();
anymore since we are using <button type="button">
HTML elements.
This ticket was mentioned in Slack in #core by desrosj. View the logs.
7 years ago
This ticket was mentioned in Slack in #gdpr-compliance by allendav. View the logs.
7 years ago
This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.
7 years ago
#12
@
7 years ago
- Keywords needs-testing added; needs-refresh removed
43929.4.diff is a refresh. Some of the action label text had changed (r43175), so please double check that for me.
Also in the refresh:
- Remove
preventDefault()
- Ensure all
<buttons>
havetype="button"
- Change translation inline comments to use
/* translators: a note */
instead of// translators: a note */
#13
@
7 years ago
Applies cleanly, tests fairly well. Two things:
"Force Remove Personal Data" should probably be "Force Erase Personal Data" for consistency.
Also, I did notice that our "Remove request" links are still anchors. (Remove is appropriate for requests, just not for personal data - that should be erase.)
#14
@
7 years ago
Thanks, @allendav. I fixed that missed text. I also added class="button"
to the remove request links, but left them as links.
Looking at the a11y handbook, it seems like these links are recommended to be <a>
tags.
If there is a valid target link for the control, either an in-page reference or a link, then the control should use an
<a href="">
@afercia could confirm. But I think this is ready to go.
#15
@
7 years ago
Some extra minor CS fixes in 43929.6.diff . Other than that it looks good to me and applies & works without issues @desrosj .
wp-admin\js\xfn.js @ 218 spaces to tabs @ 222 spaces to tabs @ 227 spaces to tabs @ 229 spaces to tabs @ 234 new line above comment @ 264 convert `} )( jQuery );` to `} ( jQuery ) );`
wp-admin\includes\user.php @ 666 remove empty line @ 1115 space before `: '';` @ 1218 space before `: 0;` @ 1480 `,` after array
wp-includes\user.php @ 3081 align `=` @ 3114 space before `: 0;`
43929.diff
Thanks @afercia for taking time to check things.
<a>
to<buttons>
where needed inuser.php
-
instead of_
inuser.php
vars
inxfn.js
user.php / xfn.js
I tested everything ( using trunk ) but please do thorough tests as all action handlers etc have been touched :]
Note: I didn't mess with non-privacy code so there are still a couple of CS notices in those files.
cc @allendav ( pinging since you know these actions better than anyone :D )