Skip to:
Content

Opened 4 years ago

Closed 3 years ago

Last modified 2 years ago

#6122 closed defect (bug) (fixed)

BP JS whats-new-options animation cuts of larger child elements

Reported by: hnla Owned by: boonebgorges
Milestone: 2.3 Priority: normal
Severity: normal Version:
Component: Templates Keywords: has-patch
Cc:

Description

This has been an issue for a while with some themes.

BP runs a jquery animation routine on the whats new update elements setting a fixed height for the textarea and the whats-new-options parent that wraps the select and submit elements.

The issue is a fixed height as been set and in the BP styles height: 0; overflow: auto; used to hide the initial state of the element on load.

The result is with certain themes where the submit button is larger than BP anticipated we see the button cut off and very ugly overflow scrollbars introduced due to the auto overflow property, in addition we do not actually have a mechanism in place that returns the options element to it's original hidden state when the .blur routine is called.

This patch attempts to rectify these issues.

Firstly it changes all animation properties 'height' to 'minHeight'
There is no real reason we should be setting fixed height, fixed height are generally considered bad unless for a very specific reason, here there is no real reason and with the overflow factor changed elements will simply push out through the parent so an issue if styling dictates a background for example.

Secondly we change the animate height property on the .blur routine to have a value of null '' thus actually removing it rather than re-stating the value '40px' now our options will hide themselves on losing focus on the textarea.

Lastly a series of classes are set to provide styling hooks for the state changes; so we have 'whats-new-show' and 'whats-new-hide' for the #whats-new-options and one for the textarea to show it's in focussed state - this is less critical but potentially useful.

The two new classes for show/hide will allow us rectify this issue by stating the overflow of the element to as 'hidden' and 'visible'

There will need to be an additional patch to BP styles to correct the issue for future themes but this patch will allow devs to easily make adjustments where necessary.

Attachments (3)

6122-01.patch (2.5 KB) - added by hnla 4 years ago.
6122-02.patch (1.8 KB) - added by hnla 4 years ago.
6122.03.patch (834 bytes) - added by r-a-y 3 years ago.

Download all attachments as: .zip

Change History (17)

@hnla
4 years ago

#1 @DJPaul
4 years ago

  • Milestone changed from 2.2 to 2.3

I am hesitant about putting any new tickets that aren't regressions into 2.2 unless it's a very narrow and specific change. I think this one is larger than I feel comfortable with, so I've moved it into 2.3.

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


4 years ago

#3 @hnla
4 years ago

  • Milestone changed from 2.3 to 2.2

6112-02 updates and corrects -01, had not updated and included previous patch in file.

@hnla
4 years ago

#4 @hnla
4 years ago

@DjPaul why are you nervous of this? I don't understand the regression nature you refer to. the patch adds essentially a series of classes, and changes to what should never have been 'height' to something that works 'min-height' it should have no effect on existing sites, and until the classes are actually referenced in styles somewhere they should not have any impact, I also took care to name then uniquely as some frameworks use .show etc.

Check some themes to see how awful this issue is when it rears it's ugly head e.g twentyfifteen, this fix has long needed adding, it's bugged me badly for a long time.

#5 @johnjamesjacoby
4 years ago

Applied these patches and, though it alleviates the problem, I don't know that I'm comfortable with it. Adding and removing classes that aren't used in core CSS seems unnecessary (though helpful for child themes) and juggling the minHeight without setting it also in the CSS seems like a gamble.

I vote to bump this to 2.3 for now, pending more discussion and a rock solid approach. Maybe we go all-out and introduce a killer "post from anywhere" box instead?

#6 @hnla
4 years ago

Adding and removing classes that aren't used in core CSS seems unnecessary (though helpful for child themes)

No the intention was and is to use those classes in the main styles for BP, it's that way we correct the issue in general, it's simply a time factor, I hadn't time to run up and test styles for BP on this against a few themes but intended to, regardless, in this respect the js aspect and classes could be available at no particular cost to anything even if not directly used.

minHeight is simply a re-factoring of what existed as height it still has height property manipulation in the stylesheet.

Ideally what ought to happen is that entire animation block be removed, it should be a site by site decision to effect behaviour like that really.

Like the idea of a 'post from anywhere' box.

#7 @DJPaul
4 years ago

  • Milestone changed from 2.2 to 2.3

Moving back to 2.3. Not trying to pull rank on you, hnla, even if it seems like that. :) I think if this patch had gone up before we got to 2.2 beta, there would have been more appettite and time to iterate/test on it.

If you still feel strongly about this for 2.2 and think we're making a mistake, please chat to either of us on Slack so we can bash this out. Thanks

#8 @hnla
4 years ago

@DjPaul I think we're making a slight mistake in not taking seriously enough the mess that arises when this aspect demonstrates it's inherent flaw, but blame myself for that as I've wrestled with this often enough and should have been more decisive much earlier on like 4 releases back :)

I do agree though in the lateness of adding something like this when in a final beta stage, that troubled me too, for the moment I can overload BP JS and maintain the file by updates and merges.

@r-a-y
3 years ago

#9 @r-a-y
3 years ago

03.patch is almost the same as hnla's patch, but without the class additions.

I've also stuck with height instead of minHeight because minHeight causes scrollbars to appear during animation (at least in Firefox).

Also 50px appears to be the safe height for no scrollbars to show.

#10 @hnla
3 years ago

@r-a-y However it is the very fact of stating height that is the weakness here in what had been done- if min-height caused issues it's likely more that styles need addressing, I saw no issues but would need to review that in case I missed something ( I had expressly tested a set of styles required as part of the overall solution).

Yes increasing the height will alleviate the immediate problem, however I'll just come along and style a theme with 60px high submits :)

The exercise was really to tackle the problem in a manner that meant we had no further issues, and that devs/themers had appropriate class tokens to hook styles on.

I would Support r-a-y's revision though as a stop gap, it will hold up for most cases and will for twentythirteen, but do dislike re-visiting things like this :(

#11 @DJPaul
3 years ago

@hnla, when we build new template parts, the sort of problem you describe will just go away. We can build things in a better way, now that we all have learnt so much in the intervening years. We just need smallish fixes for bp-legacy to help it grow old gracefully.

#12 @boonebgorges
3 years ago

The perfect is the enemy of the good. Let's go with this.

#13 @boonebgorges
3 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 9829:

Better spacing for submit button when doing What's New animation.

The extra breathing room ensures that we don't see scroll bars or a button
that is cut off.

Props hnla, r-a-y.
Fixes #6122.

#14 @DJPaul
2 years ago

  • Component changed from Appearance - Template Parts to Templates
Note: See TracTickets for help on using tickets.