-
Notifications
You must be signed in to change notification settings - Fork 42
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
base: trunk
Are you sure you want to change the base?
Conversation
@JJJ can you please review this pull request. Let me know if I am missing anything. |
There was a problem hiding this 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 besanitize_key()
instead, or evensanitize_css_class()
depending...
It's really too bad the CSS classes aren't more abstracted here. I can totally imagine customizing them being useful.
Thanks @JJJ |
There was a problem hiding this 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.
There was a problem hiding this 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. 👍
There was a problem hiding this 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.
Thanks @mikachan for the review :) I think this PR is ready to commit. |
Fixes 2692