#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)
Change History (17)
This ticket was mentioned in Slack in #buddypress by hnla. View the logs.
10 years ago
#3
@
10 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.
#4
@
10 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
@
10 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
@
10 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
@
10 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
@
10 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.
#9
@
10 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
@
10 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
@
10 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.
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.