Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 7 years ago

#7443 closed defect (bug) (fixed)

Hidden groups CSS specificity

Reported by: mikegillihan's profile MikeGillihan Owned by: r-a-y's profile r-a-y
Milestone: 2.9 Priority: normal
Severity: minor Version: 1.7
Component: Groups Keywords: has-patch
Cc:

Description

The "hidden" class applied to a Hidden group in the Groups list is too generic. The .hidden pattern is fairly widespread and anytime a user with that pattern in a theme/plugin tries to view their groups, the Hidden groups will not be displayed appropriately.

While I don't really think it is a bug, it doesn't appear that there is a presentational reason for having that class so changing it to something like "hidden-group" would be more appropriate IMHO and eliminate the conflicts.

Attachments (1)

7443.js.patch (641 bytes) - added by r-a-y 7 years ago.

Download all attachments as: .zip

Change History (14)

#1 @r-a-y
8 years ago

  • Component changed from Core to Groups
  • Keywords dev-feedback added
  • Type changed from enhancement to defect (bug)
  • Version changed from 2.7.2 to 1.7

Related support thread:
https://buddypress.org/support/topic/hidden-groups-and-forums-not-showing-for-members-of-them/


Yeah, I can see the .hidden CSS rule being used by other themes here.

If we make the change to the hidden CSS class to something else like hidden-group, we'll probably break sites that are already using this rule to style hidden groups in Groups loop.

I think this is important to fix though.

We could do what other people have done in the support thread and that is to use a display CSS override using !important. This wouldn't break any sites, but I know @hnla will hate this option :)

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


8 years ago

#3 @danbp
8 years ago

@r-a-y, @hnla

IMHO, this is only an expectation "we'll probably break sites that are already using this rule" and is at least at the same level as breaking sites using the actual default rule (and the reason of this ticket).

Would you name a class .relative or .width ? Probably not. So why would you continue to use .hidden ?

This shouldn't be a long decision, especially because it exist a CSS naming convention.

#4 @hnla
8 years ago

So @danbp you're arguing to change the class altogether? and be damned those that chose hang styles on it, inclined to not disagree but on the fence.

How about a further option:
retain the class 'hidden' to retain styles hooked already to it, add a new class '.bp-group-hidden' to that add the styles we need with a little specificity if necessary to ensure the properties are favoured, not not perhaps ideal but...

I'll go with the consensus opinion on the fix, whilst running over to Nouveau templates to try and make sure it doesn't occur there :)

#5 @henry.wright
8 years ago

If a theme is using .hidden { display: none; } would !important be absolutely necessary in an upstream style sheet? I think greater specifity would be enough. For example #group .hidden { display: block; }

#6 @MikeGillihan
8 years ago

Thanks for the quick replies.

I'm not sure anything other than removing/replacing the hidden class would resolve this ticket, but I don't think I could call this a bug in BP either. In my opinion, the CSS pattern itself is pretty shortsighted and shouldn't be used, especially with the !important tag.

In the project I'm working on I added a quick override using !important because the theme employs many of H5BP's styles. I'm pretty sure bootstrap used the same pattern at some point so I figured tracking the issue was worth it.

Not being knee deep in BuddyPress I'm not sure what ramifications removing the class would have functionally. It seems to be a bit of a damned if you do, damned if you don't situation.

Again, thanks for the response and listening to me ramble. :)

#7 @boonebgorges
8 years ago

  • Keywords close added

@MikeGillihan Thanks very much for the report - I've run into this before as well. As you note, it's bad practice - both by BP and by the theme that applies !important rules to .hidden elements.

I'll go with the consensus opinion on the fix, whilst running over to Nouveau templates to try and make sure it doesn't occur there :)

Unfortunately, I think this is probably the best we can do, without breaking existing sites in ugly ways.

#8 @r-a-y
7 years ago

  • Keywords has-patch added; dev-feedback close removed
  • Milestone changed from Awaiting Review to 2.9

Moving this to 2.9.

Bootstrap uses the .hidden CSS class, which conflicts with the .hidden class we use in group lists.

I would recommend the following CSS declaration to fix this:

#groups-list li.hidden {
	display: list-item !important;
}

This would probably minimize the amount of breakage, however it might cause problems in custom BP themes that are using display: inline-block or some other variation.

In that case, I've also attached a JS fix for this.

@r-a-y
7 years ago

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


7 years ago

#10 @hnla
7 years ago

@r-a-y I think the JS solution may be preferable and better inherited by themes that may have overloaded styles?

#11 @hnla
7 years ago

@r-a-y Should we pop this in then, or punt?

#12 @r-a-y
7 years ago

I'm going to commit the JS approach for now.

Thanks for the poke, @hnla!

#13 @r-a-y
7 years ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

In 11613:

bp-legacy: Fix the display of hidden groups in group loops in some instances.

Themes or plugins that use the .hidden CSS declaration to hide content
can conflict with hidden groups in group loops. Themes that utilize the
Bootstrap framework are particularly prone to this problem.

The issue is due to us using the group status as a CSS class and 'hidden'
is a valid BuddyPress group status option.

To workaround this, we check the visibility of a hidden group in the group
loop with JS. If the group is hidden via CSS, we force the display of the
group with the !important rule.

In the future, we should probably deprecate the usage of the hidden CSS
class in group loops for something else.

Props r-a-y, MikeGillihan, hnla, henry.wright.

Fixes #7443.

Note: See TracTickets for help on using tickets.