Skip to:

Opened 7 years ago

Last modified 6 years ago

#7548 new enhancement

bp_button_class needs to know what $container value is to set correct $parent_element

Reported by: hnla's profile hnla Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Core Keywords: 2nd-opinion early


This follows on from #7532

With Nouveau able to set args for button elements parent wrappers e.g ul, div etc, nouveau hardcoded the $parent_element for the button or anchor to one of 'li' if $container had a value of 'ul'

The problem arises that third party plugins may filter directly to the button class in BP without any means to know their anchor is rendering in a list construct and thus break that construct with an illegal element in the UL.

Proposal & patch to follow as time allows is to pass into the Nouveau args array that $container value so that the BP class has it available then to run a similar check as in Nouveau to replace the 'div' wrapper with a 'li' one if that $container has a value of 'UL' this way we avoid issues such as the 'User Switcher' plugin has.

Change History (8)

#1 @r-a-y
7 years ago

  • Keywords 2nd-opinion added; needs-patch removed

I'm not convinced this is a core issue (yet)!

Can you create a Nouveau ticket and CC me on it?

#2 @hnla
7 years ago

  • Keywords early added
  • Milestone changed from 2.9 to 3.0

It's a core issue in that we re-factored core button class to accept changes to the element but while hardcoding a wrapper element, given Nouveau allows both to be modified we should provide a simple means to allow the button class to check what if any $container value set and switch elements, it seems to me to be something inconsequential for the class to maintain.

However for this to work means I have to re-visit the Nouveau button args to pass $container into the arrays so BP button class has it available. For the moment I'm punting this to 3.0 I'll work up a patch then for it.

Can you create a Nouveau ticket and CC me on it?

Think it was noted there but not sure a ticket on Nouveau makes huge sense, maybe though, if not existing I'll circle back around later.

#3 @DJPaul
6 years ago

Do we have to do the proposed in this ticket - or can we trust plugin authors to set HTML elements correctly for their extensions?

#4 @DJPaul
6 years ago

@hnla please respond when you have time.

#5 @DJPaul
6 years ago

  • Milestone changed from 3.0 to Under Consideration

#6 @hnla
6 years ago

I believe this was the only solution I could arrive at for this potential problem where plugins could assume one thing (div, a) while Nouveau might have changed to a (ul, li) - We can't trust plugin authors as it's unfair for them to have to know, in the example of User Switcher it renders the markup it believes is ok, Nouveau can now upset that assumption. Let me look again at this when I have a moment.

#7 @DJPaul
6 years ago

Given the amount of open tickets, I am very inclined to not want to spend time fixing this for now until it becomes a problem.
We've got a lot of immediate issues to resolve, rather than worry about potential problems.

#8 @DJPaul
6 years ago

  • Milestone Under Consideration deleted

No action in 4 months, closing for now. Re-open if/when we get a chance to re-assess the issue and prioritise it appropriately, thank you. :)

Note: See TracTickets for help on using tickets.