Opened 8 weeks ago
Closed 3 days ago
#9308 closed enhancement (fixed)
Risky: bp-groups-template.php adds ".hidden" class.
| Reported by: |
|
Owned by: |
|
|---|---|---|---|
| Milestone: | 14.5.0 | Priority: | normal |
| Severity: | normal | Version: | |
| Component: | Groups | Keywords: | |
| Cc: | emaralive |
Description
In line 730 of bp-groups-template.php a status class is added to a listed group.
// Group type - public, private, hidden.
$classes[] = sanitize_key( $groups_template->group->status );
A class like "hidden" is so generic it may be affected by theme or plugin CSS that may define, rightly or wrongly, a broadly scoped ".hidden"-class that, in turn, may have the unintentional effect of visually hiding groups that should be visible to the user.
I suggest adding a prefix to the status class like "status-" to avoid this problem.
// Group type - public, private, hidden.
$classes[] = 'status-' . sanitize_key( $groups_template->group->status );
Attachments (2)
Change History (11)
#3
@
3 weeks ago
I agree with this @dcavins concern. Using a generic class name like .hidden is risky because itβs very likely to conflict with theme or framework CSS (Bootstrap, etc.).
Iβd be in favour of introducing prefixed classes like group-status-public, group-status-private, and group-status-hidden, even if that means a minor adjustment period for existing custom CSS. A short note in the release notes would be enough for developers to update their styles.
#4
@
7 days ago
- Milestone changed from Awaiting Review to 14.5.0
- Type changed from defect (bug) to enhancement
I like @noruzzaman approach, even though I agree and understand @dcavins's concern. Let's try to give as much notice as possible.
We could introduce the new prefixed classes now and remove the non-prefixed ones in a major version.
#5
@
4 days ago
Created a patch implementing the backwards-compatible approach discussed above.
The fix adds a prefixed group-status-{status} class (e.g., group-status-hidden, group-status-public, group-status-private) while keeping the old non-prefixed class for backwards compatibility. Added a @todo comment noting the non-prefixed class should be removed in a future major version.
This gives theme/plugin authors time to update their CSS selectors while preventing the Bootstrap .hidden trap from affecting new installations.
Thanks @t.schwarz for the report and @dcavins, @noruzzaman for the discussion.
#6
@
3 days ago
@vapvarun Thanks for the patch. I'll make one small change to the classes. I'll use 'bp-group-status-' . $group_status; just for consistency and better CSS specificity. :)
#7
@
3 days ago
@espellcaste Makes sense - updated to use bp-group-status- prefix. Ready when you are.
βhttps://github.com/vapvarun/BuddyPress/commit/db35544920d19101246ec2a487baaecfaf06288d
This is a good point. I think of it as the bootstrap trap, where bootstrap uses the most generic class names possible and conflicts with nearly everything. :)
The downside of this change is that if people are targeting the .hidden class, their styles will miss their targets. I can't think of a way to introduce this change without messing up a few people. I mean, we could add
group-status-publicandpublicfor a version to allow people who actually read the release notes to make the update smoothly, but that's not a large group of people.