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

Css class added for user roles in topics and replies #6

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

robinwpdeveloper
Copy link

Fixes 2692

@robinwpdeveloper
Copy link
Author

@JJJ can you please review this pull request.
This is my first contribution to bbPress. :)

Let me know if I am missing anything.

Copy link
Contributor

@JJJ JJJ left a comment

Choose a reason for hiding this comment

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

Hello @robinwpdeveloper! 👋 This is an awesome first attempt!

I see what you are wanting here, and I think it is a good idea.

Because of how these functions are currently coded to work, there are a few different parts of your approach that could use refinement.

  • Calling functions before parsing arguments is (generally) a pattern that we (almost always try to) avoid whenever possible.
  • bbp_get_user_display_role() now gets called twice, before and after arguments are parsed
  • I wonder if your strtolower() call should be sanitize_key() instead, or even sanitize_css_class() depending...

It's really too bad the CSS classes aren't more abstracted here. I can totally imagine customizing them being useful.

@robinwpdeveloper
Copy link
Author

robinwpdeveloper commented Sep 7, 2022

Thanks @JJJ
I have made changes accordingly as per your suggestion.
Let me know if any more changes are needed.

Copy link

@rudlinkon rudlinkon left a comment

Choose a reason for hiding this comment

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

sanitize_key function itself used strtolower function so we do not need to do it again.

src/includes/replies/template.php Outdated Show resolved Hide resolved
src/includes/topics/template.php Outdated Show resolved Hide resolved
Copy link

@rudlinkon rudlinkon left a comment

Choose a reason for hiding this comment

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

thank you @robinwpdeveloper it looks good to me. 👍

Copy link

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

This works well for me 👍

I've left some tiny suggestions but otherwise, I think this is ready to merge.

src/includes/replies/template.php Outdated Show resolved Hide resolved
src/includes/topics/template.php Outdated Show resolved Hide resolved
@robinwpdeveloper
Copy link
Author

Thanks @mikachan for the review :)
Requested changes are made and pushed.

I think this PR is ready to commit.

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.

4 participants