WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#38755 closed defect (bug) (fixed)

Customize: use calc() to set textarea height for custom CSS

Reported by: michaelarestad Owned by: michaelarestad
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Customize Keywords: has-patch
Focuses: ui Cc:
PR Number:

Description

The custom CSS module is pretty rad, but right now the height is just fixed at 553px. I think we can do better by making it as large as it can be in the visible space.

The method I propose uses a combination of the calc() function and vh, a viewport unit. If the browser doesn't support either calc() or vh, it will gracefully fall back to whatever we set the fallback height to.

My first patch fills as much space as possible. I have a minor concern that when other plugins enhance this editor (Jetpack), that the controls for it will be below and might be lost because it is filling the space. That said, those plugins could just override the height.

Attachments (8)

38755.diff (417 bytes) - added by michaelarestad 3 years ago.
38755.2.diff (433 bytes) - added by michaelarestad 3 years ago.
scrollbar.png (64.7 KB) - added by westonruter 3 years ago.
38755.3.diff (1.0 KB) - added by michaelarestad 3 years ago.
padding-margin-issues.png (63.5 KB) - added by westonruter 3 years ago.
38755.4.diff (2.4 KB) - added by westonruter 3 years ago.
https://github.com/xwp/wordpress-develop/commit/f29a50d
custom-css-desktop.png (61.4 KB) - added by westonruter 3 years ago.
custom-css-mobile.png (64.1 KB) - added by westonruter 3 years ago.

Download all attachments as: .zip

Change History (20)

@michaelarestad
3 years ago

#1 @michaelarestad
3 years ago

  • Keywords has-patch added
  • Owner set to michaelarestad
  • Status changed from new to accepted

#2 @michaelarestad
3 years ago

I changed the textarea to resize: none since it's not possible to resize when a height is set. If we used min-height, it would be resizable, but if it's as large as can be on a screen, I'm not sure why it would be necessary.

@westonruter
3 years ago

#3 @westonruter
3 years ago

  • Milestone changed from Awaiting Review to 4.7
  • Type changed from enhancement to defect (bug)

@michaelarestad good idea. However, as you can see in scrollbar.png, the scrollbars are still appearing. Shouldn't they be hidden entirely in the normal case?

#4 @michaelarestad
3 years ago

@westonruter good catch. Looking into it.

#5 @michaelarestad
3 years ago

@westonruter I think I got rid of the scrollbar in the latest patch until the textarea gets full and then it's a normal scrollbar. Can you confirm?

It's also responsive, now.

#6 @westonruter
3 years ago

  • Keywords needs-patch added; has-patch removed

@michaelarestad yeah, I can confirm that the scrollbars don't show up normally now. However, I see one defect and one possible improvement: the padding in the section heading now seems to be cut at the top now in the Custom CSS section whereas it has the expected amount of whitespace (more) in the other sections.

Also, do you think the margin at the bottom could be removed?

Also, thinking about integrating with Jetpack where it will add additional controls to this section (see #38672, cc @georgestephanis), how about if the margin at the bottom is removed if it is the last item in the list, for example: li.customize-control-custom_css:last-child { margin-bottom: 0; }

And also, the full-section height of the textarea should likewise probably only be done if there is only one control in the section. So instead of the #sub-accordion-section-custom_css textarea selector, what about: .customize-section-description-container + #customize-control-custom_css:last-child. This will ensure it is the first and last control in the section.

#7 @georgestephanis
3 years ago

We're hiding the original input, regardless. The Codemirror element just syncs its content back and forth with the hidden original input -- so whatever core does to its styling shouldn't bother us much.

#8 @westonruter
3 years ago

  • Keywords has-patch added; needs-patch removed

@michaelarestad how does 38755.4.diff look? It should address my feedback.

#9 follow-up: @celloexpressions
3 years ago

As with any customizer section, the CSS section cannot assume that there are no non-core controls added. We definitely need to only auto-size to the full size when the core control is the only one in the section; and even then, I'm not sure that showing a larger textarea provides usability benefits. The area auto-resizes based on its contents (at least, last time I checked), and an overly large input may imply that it's intended to contain much more CSS than it reasonably might for the average user, causing a level of intimidation. It could be a nice enhancement, but I'm not convinced yet. I'm certainly skeptical of this sort of change at this point in the release cycle.

Also, for any changes to this feature and extensibility considerations, do remember that there are hundreds of CSS plugins and even more CSS options in themes and plugins that will need to integrate with the new core option, in addition to Jetpack.

#10 in reply to: ↑ 9 @westonruter
3 years ago

Replying to celloexpressions:

As with any customizer section, the CSS section cannot assume that there are no non-core controls added.

Did you try 38755.4.diff? It specifically only does the full-height behavior if the section only has the one control.

#11 @delawski
3 years ago

@westonruter I have reviewed the patch and it looks great. I've tested it out locally in both cases:

  • when there is only the CSS editor, and
  • when there is some other control besides of the editor.

The patch works as expected in either scenario. I think it's ready to merge.

#12 @westonruter
3 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 39229:

Customize: Maximize height of Custom CSS textarea without causing doubled scrollbars.

Textarea will fill vertical height of Custom CSS section for browsers that support calc() and when plugins don't add other controls to the section. Also run CSS autoprefixer.

Props michaelarestad, westonruter.
See #35395.
Fixes #38755.

Note: See TracTickets for help on using tickets.