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

Various tweaks #118

Merged
merged 20 commits into from
Jan 23, 2024
Merged

Various tweaks #118

merged 20 commits into from
Jan 23, 2024

Conversation

aristath
Copy link
Member

@aristath aristath commented Jan 23, 2024

Context

This PR contains various tweaks and improvements:

  • Removes the grunt task. Our CSS & JS files are so small and basic that we don't need build processes, minification etc.
  • Fixes errors in VSCode due to PHPCompatibility checks (tweaked the PHPCS ruleset)
  • Code readability tweaks, especially in places where we print HTML in admin pages. The structure didn't change there, just separating long lines etc.
  • Made a few strings translatable
  • Fixed a bug where a variable was undefined
  • Changed the enqueue functions to point to the correct JS & CSS files
  • Replaced the yst prefix with epch (Emilia Projects Comment Hacks)
  • Removed the Composer autoloader and added a new custom one with a hardcoded classmap

Relevant technical choices:

Test instructions

  • Activate the plugin - ensure there are no fatalities in the autoloader
  • Go the the admin page - check that the page appears correctly and tabs work as expected

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Block/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities.
  • I have added unit tests to verify the code works as intended.
  • I have checked that the base branch is correctly set.

Fixes #

@aristath aristath changed the base branch from trunk to develop January 23, 2024 09:53
Comment on lines -190 to +213
$roles = \apply_filters( 'EmiliaProjects\WP\Comment\notification_roles', $roles );
$roles = \apply_filters(
'EmiliaProjects\WP\Comment\notification_roles',
[ 'contributor', 'author', 'editor', 'administrator', 'super-admin' ]
);
Copy link
Member Author

Choose a reason for hiding this comment

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

$roles was undefined here. The doc for this filter was saying "Defaults to contributor and up", so the array used here contains these roles.

@aristath aristath marked this pull request as ready for review January 23, 2024 10:59
@aristath aristath requested a review from jdevalk January 23, 2024 10:59
@aristath aristath mentioned this pull request Jan 23, 2024
9 tasks
Copy link
Member

@jdevalk jdevalk left a comment

Choose a reason for hiding this comment

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

Couple of remarks above.

I also think we should update composer.json to be all Emilia not joost.blog?

admin/assets/css/comment-hacks.css Outdated Show resolved Hide resolved
admin/views/comment-parent-box.php Outdated Show resolved Hide resolved
inc/clean-emails.php Outdated Show resolved Hide resolved
inc/clean-emails.php Outdated Show resolved Hide resolved
Copy link
Member

@jdevalk jdevalk left a comment

Choose a reason for hiding this comment

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

Few more comments.

.phpcs.xml.dist Show resolved Hide resolved
.phpcs.xml.dist Outdated Show resolved Hide resolved
@aristath
Copy link
Member Author

I also think we should update composer.json to be all Emilia not joost.blog?

I changed the homepage to point to https://emilia.capital. The authors array however has your name and personal blog, I don't think we have to change that, should be fine to keep it since it describes the author 🤔

admin/admin.php Outdated Show resolved Hide resolved
admin/admin.php Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Jan 23, 2024

WordPress Playground

You can easily test this pull request on the WordPress Playground.

@jdevalk jdevalk merged commit 100ec2b into develop Jan 23, 2024
13 checks passed
@jdevalk jdevalk deleted the various-tweaks branch January 23, 2024 13:16
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.

2 participants