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

Modify checklist in PR template #1322

Merged
merged 2 commits into from
Jun 2, 2022
Merged

Modify checklist in PR template #1322

merged 2 commits into from
Jun 2, 2022

Conversation

osma
Copy link
Member

@osma osma commented May 6, 2022

Reasons for creating this PR

After using the PR template for a while, I've found that the checklist is a bit problematic, especially the question about added unit tests. This PR rephrases the question so that it can be ticked also in cases where tests are not relevant (e.g. only changes to front-end code for which we don't have unit tests anyway)

Link to relevant issue(s), if any

n/a

Description of the changes in this PR

  • rephrase the 2nd checklist question about unit tests
  • add a new checklist question that the PR doesn't harm accessibility (suggested by @kouralex )

Known problems or uncertainties in this PR

n/a

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if not, explain why below)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@osma osma added this to the 2.15 milestone May 6, 2022
@osma osma self-assigned this May 6, 2022
@@ -11,5 +11,6 @@
## Checklist

- [ ] phpUnit tests pass locally with my changes
- [ ] I have added tests that prove my fix is effective or that my feature works (if not, explain why below)
- [ ] I have added tests that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
- [ ] The PR doesn't reduce accessibility of the front-end code
Copy link
Collaborator

Choose a reason for hiding this comment

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

👏 🙇

@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #1322 (042500e) into master (a14435f) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #1322   +/-   ##
=========================================
  Coverage     70.68%   70.68%           
  Complexity     1646     1646           
=========================================
  Files            32       32           
  Lines          3786     3786           
=========================================
  Hits           2676     2676           
  Misses         1110     1110           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a14435f...042500e. Read the comment docs.

@kouralex kouralex self-requested a review May 6, 2022 08:02
Copy link
Contributor

@kouralex kouralex left a comment

Choose a reason for hiding this comment

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

Very nice and needed changes/additions @osma !

Minor comments:
One could add examples to the newly added accessibility part. E.g., tabbing works (element flow is logical, focused element is visually distinguishable), .sr-only is used when approriate (together with appropriate usage of headings), color contrast is high enough... EDIT: also that it scales well (different zoom/resolution levels)

Other than that, looks good to me! 👍

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@osma
Copy link
Member Author

osma commented May 6, 2022

One could add examples to the newly added accessibility part. E.g., tabbing works (element flow is logical, focused element is visually distinguishable), .sr-only is used when approriate (together with appropriate usage of headings), color contrast is high enough... EDIT: also that it scales well (different zoom/resolution levels)

I made small fixes in a new commit based on your suggestions, but I think it's impossible to give a thorough lesson on accessibility in the context of a single checkbox so these are by necessity just reminder keywords, intended for a developer who knows about accessible design.

@kouralex kouralex self-requested a review May 6, 2022 13:11
@osma osma merged commit a54bada into master Jun 2, 2022
@osma osma deleted the osma-patch-1 branch June 2, 2022 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants