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

Improvement: Implement accessibility statement and feedback mechanism, resolves #567. #707

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

detomon
Copy link
Collaborator

@detomon detomon commented Aug 29, 2024

Resolves #567.

@detomon detomon force-pushed the accessibility-statement-and-feedback-mechanism branch 2 times, most recently from 2b367b0 to fdbd177 Compare September 16, 2024 11:00
@abias abias force-pushed the main branch 3 times, most recently from 5696e9a to b9fa4ca Compare October 21, 2024 15:33
@detomon detomon force-pushed the accessibility-statement-and-feedback-mechanism branch from fdbd177 to 5614e65 Compare October 28, 2024 14:45
@christianwolters
Copy link
Member

Hi @detomon,

I'd like to review your pull request but there are unrelated commits in this PR.
(maybe a rebase went wrong?)

Can you please cleanup your PR by resetting your branch to current main and cherry-pick your main commit?

Cheers
Christian

@detomon detomon force-pushed the accessibility-statement-and-feedback-mechanism branch from 5614e65 to 97413cd Compare November 20, 2024 15:17
@detomon
Copy link
Collaborator Author

detomon commented Nov 21, 2024

Hi @christianwolters,

It seems that main has been rebase at some point. Should be ok now.

Thank you for taking the time to review our pull request!

Copy link
Member

@christianwolters christianwolters left a comment

Choose a reason for hiding this comment

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

Hi @detomon,

thank you and your coworkers for this contribution. I really like seeing a11y getting a united boost here (sorry ^^)

My manual user facing tests checked out, during code review I found a few spots that still need some work:

  • I commented directly in code as you should see above

Furthermore, please take a look at your version.php

  • you are introducing new settings, so please increment the value of $plugin->version

Cheers
Christian

@christianwolters
Copy link
Member

christianwolters commented Dec 3, 2024

Review

Following https://github.com/moodle-an-hochschulen/moodle-plugin-maintaining/wiki/Check-list-for-peer-reviewing-patches-and-pull-requests

Documentation

  • Commit Message understandable and issue referenced (via resolves keyword)
  • Patch author correct
  • CHANGES.md appended
  • README.md appended
  • Credits appended
  • Appropriate Comment quantity
  • Successful Moodle PHPDoc check

version.php

  • Checked if $plugin->version increment necessary (and increment done if necessary)
  • $plugin->release untouched

lib.php

  • Only necessary functions

Languages

  • Only english strings
  • Necessary magic strings added (e.g. capabilities)

Automated tests

  • New functionality covered
  • No failing steps or scenarios

Mustache templates

  • Example context exists

CSS and styles.css

  • Bootstrap styles used if possible

Duplicated code

  • Duplicated code is marked properly

Github action

  • All green

Commit history and scope

  • Already single commit
  • Focus on single purpose
  • No surplus files

Additional aspects for Boost Union

  • No usage of $theme->settings
  • Usage of isset() checks when processing plugin settings in renderer / output code
  • Usage of admin_setting_configselect instead of admin_setting_configcheckbox
  • Modified mustache templates properly marked and .upstream template in place

Review result

Hi @detomon, these are the results of my review. (I forgot to append the checklist)

See comments and conversation above, open checks:

  • Appropriate Comment quantity
  • Checked if $plugin->version increment necessary (and increment done if necessary)

Do you have time in the next weeks to address the open points?

Cheers
Christian

@detomon detomon force-pushed the accessibility-statement-and-feedback-mechanism branch from 97413cd to fe3cfeb Compare December 3, 2024 13:18
@detomon
Copy link
Collaborator Author

detomon commented Dec 3, 2024

Hoi @christianwolters,

I just pushed the changes regarding the missing points.

Cheers,
Simon

@christianwolters
Copy link
Member

Hi @detomon,

thank you for your changes, this looks good.

As this is PR involces changes in many files, I'll now hand over to @abias for a second review and integration.

Cheers
Christian

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.

accessibility statement and feedback mechanism
2 participants