WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 2 years ago

#19155 closed defect (bug) (fixed)

Remove scrollbar and max-height from help tabs

Reported by: ocean90 Owned by: koopersmith
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.3
Component: Administration Keywords:
Focuses: Cc:

Description

In Firefox there are two scrollbars: http://cl.ly/BZQg
In Chrome is only one for all columns: http://cl.ly/BYJE

Attachments (2)

help.png (52.3 KB) - added by olleicua 2 years ago.
19155.diff (766 bytes) - added by sorich87 2 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 ocean902 years ago

  • Status changed from new to assigned

Okay, in Chrome the window was wide enough, so the text fit into the box. But the scrollbar for the links is still there.

olleicua2 years ago

comment:2 follow-up: olleicua2 years ago

  • Cc olleicua@… added

My scrollbars in FF7 are in a completely different place see help.png. Wouldn't it make the most sense to just not define a maximum height for the help panels so that it is as tall as it needs to be, no scrolling necessary. This seems like it would be a bit friendlier to me.

comment:3 in reply to: ↑ 2 jane2 years ago

Replying to olleicua:

My scrollbars in FF7 are in a completely different place see help.png. Wouldn't it make the most sense to just not define a maximum height for the help panels so that it is as tall as it needs to be, no scrolling necessary. This seems like it would be a bit friendlier to me.

Yes, the intention was for no scrolling... that was the point of breaking up help content into tabs in the first place. Removing max height good with me.

comment:4 ocean902 years ago

  • Keywords needs-patch added

olleicua, the problem is, that the height isn't calculated new. max-height is 300px, and on your screen aren't 300px. Could reproduce it here too.

comment:5 chexee2 years ago

So, per what Jane said, there shouldn't be any scrollbars here. I poked around and just removing the max-height doesn't do it (looks like the heights are calculated and explicitly set in the JS).

Could we remove max height and make the help height animate and expand to longer lengths of text and you roll through the side tabs?

Last edited 2 years ago by chexee (previous) (diff)

sorich872 years ago

comment:6 sorich872 years ago

The issue was that the height was not properly recalculated when navigating through the panels . The initial height that was previously set when you clicked on the help was just kept.

19155.diff resets the height before recalculating it again. In addition, it also removes max-height.

comment:7 sorich872 years ago

  • Keywords has-patch added; needs-patch removed

comment:8 koopersmith2 years ago

  • Status changed from assigned to accepted
  • Summary changed from Scrollbar in help tab looks odd to Remove scrollbar and max-height from help tabs

This works, but setting the heights via JS is no longer necessary. It can all be done via CSS.

comment:9 koopersmith2 years ago

In [19361]:

Help tabs: remove scrollbar and max-height. see #19155.

comment:10 follow-up: koopersmith2 years ago

  • Keywords ui-feedback added; has-patch removed

I've left this ticket open because I'm not sure whether or not we should animate between the changing panel heights.

That said, the bug has been fixed. Hooray!

comment:11 olleicua2 years ago

Considering it animates initially it would be more consistent to animate it always. It doesn't look bad and it's definitely way better but I think it would be best to animate it..

I was trying to look into this just now but it seems the fix has been committed since It works correctly (although without animation). However when I look at the wp-admin/js/common.dev.js the fix isn't there. I tried grep -r "columns.height" * (since columns.height is what was added in the patch) but nothing turns up. Did someone refactor this without posting here..? If so where is the code that currently changes the size so we can animate it? I'm probably missing something obvious here..

comment:13 in reply to: ↑ 10 ; follow-up: jane2 years ago

Replying to koopersmith:

I've left this ticket open because I'm not sure whether or not we should animate between the changing panel heights.

I would just as soon not.

comment:14 jane2 years ago

  • Keywords ui-feedback removed

comment:15 in reply to: ↑ 13 olleicua2 years ago

Replying to jane:

I would just as soon not.

It seems inconsistent not to to me. I'm curious why?

comment:16 jane2 years ago

Because gratuitous animation makes things feel slower. Animation should only be done if there's a purpose to it. The tab sliding down is reinforcing position and where the tabs live to the user's brain. If we're just changing height based on content, there's no real purpose that trumps speed. If I'm overlooking something, am open to being convinced otherwise.

comment:17 olleicua2 years ago

  • Keywords ui-feedback added

Either way seems has advantages. The slow down for the animation is a valid reason not to. It just seems awkward to me if it slides down and then jumps erratically once it's down there. Perhaps we should get a third opinion, I'll put UI-feedback back on for now.

comment:18 jane2 years ago

FYI, the ui-feedback tag is generally shorthand for, "Jane, please sign off on this." Anyone is welcome to leave an opinion on how to make the ui, but that tag won't necessarily make that happen.

comment:19 olleicua2 years ago

That makes sense, sorry I'm new.

comment:20 koopersmith2 years ago

  • Keywords ui-feedback removed
  • Resolution set to fixed
  • Status changed from accepted to closed

I agree with Jane here — in this case, there's no benefit to the animation, only delay. As for consistency, most of our other tabs and features that shift box heights change instantly. The animation when opening screen options/help is the exception, not the rule.

I'm going to close this ticket as fixed.

Replying to olleicua:

I was trying to look into this just now but it seems the fix has been committed since It works correctly (although without animation). Did someone refactor this without posting here..?

Yes, instead of the JS patch, I reworked it to use CSS as mentioned in comment 8 and implemented in [19361] (shown here in comment 9).

The help columns are arranged in a way that will increase the height of the help container to the height of the tallest column (regardless of that column's height), so code to calculate the column height is unnecessary.

The tricky part here was getting the three columns to each appear to stretch to the bottom of the help container. That is now accomplished by an absolutely positioned div that sits behind the three columns and paints the middle column/borders.

Replying to olleicua:

That makes sense, sorry I'm new.

Trac keywords definitely take some getting used to. We use them to manage our workflow — you'll get the hang of them soon. :)

Note: See TracTickets for help on using tickets.