Make WordPress Core

Opened 13 years ago

Closed 2 years ago

Last modified 2 years ago

#17851 closed enhancement (fixed)

Wrapping Sections with add_settings_section

Reported by: griffinjt's profile griffinjt Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version: 3.1.3
Component: Administration Keywords: has-patch good-first-bug has-unit-tests needs-dev-note commit
Focuses: Cc:

Description

This is my first time reporting, so excuse my ignorance. I just wanted to see about enhancing the add_settings_section function.

As of now, individual sections are not wrapper in any sort of container, which makes no sense to semantic sense to me. Sections should/need to be styled differently, but as of now, you really don't have much control of that.

I propose something similar to the register_sidebar function, looking like this:

add_settings_section( $id, $title, $callback, $page, $args )

$args would accept 3 parameters: before_section, after_section, and section_class.

This way you can style each individual section with relative ease. Just a thought and enhancement to the Settings API.

Thomas

Attachments (8)

17851.diff (1.1 KB) - added by scribu 13 years ago.
17851.2.diff (1.2 KB) - added by scribu 13 years ago.
Added settings-field class
template.php.patch (2.2 KB) - added by ross_ritchey 9 years ago.
Add new $args to add_settings_section. Update add_settings_section to include new $args in array. Add wrapper around printout in do_settings_section.
template.php.2.patch (1.7 KB) - added by ross_ritchey 8 years ago.
Refresh of patch
template.php.3.diff (3.8 KB) - added by palmiak 6 years ago.
Refreshed template.php file
#17851.patch (1.5 KB) - added by rehanali 3 years ago.
Added patch
17851.4.diff (2.8 KB) - added by costdev 2 years ago.
Patch #17851.patch refreshed against trunk, updated with coding standards, docblock updates, and the new, but unused, $section variable is now used.
17851.5.patch (6.7 KB) - added by martin.krcho 2 years ago.
This is updated patch 17851.4.diff with added unit tests.

Download all attachments as: .zip

Change History (46)

#1 @nacin
13 years ago

The goal of the settings API is to relieve the developer of any choices of markup and style. Ideally, the core UI styles should be used wherever possible. It also keeps us flexible -- As an example, we plan to kill the table markup in 3.3.

#2 @griffinjt
13 years ago

I understand. I just thought it would be nice for UI (e.g. jQuery accordion to keep everything on one page and manageable for lots of options)

Last edited 13 years ago by griffinjt (previous) (diff)

#3 @nacin
13 years ago

If you need an accordion to keep your options visibly in check, then you probably have too many options :-)

#4 @griffinjt
13 years ago

Tushay. :)

#5 @nacin
13 years ago

This is something that might end up happening in #16413.

I think it would be fine to just have a wrapping div with an ID generated from the section ID.

#6 @griffinjt
13 years ago

Easy and functional. I'll look into that then :)

#7 @scribu
13 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

I like the idea of automatically adding an id attribute.

Closing as duplicate of #16413

#8 @scribu
13 years ago

  • Component changed from General to Administration
  • Keywords needs-patch 3.3-early added; ui-feedback removed
  • Milestone set to Future Release
  • Resolution duplicate deleted
  • Status changed from closed to reopened

On second thought, #16413 seems pretty messy.

Automatically wrapping sections in a div and adding id attributes to field rows is a piece of cake.

#9 @griffinjt
13 years ago

That would be perfect. Just something to differentiate between sections is all that is needed :)

@scribu
13 years ago

#10 @scribu
13 years ago

  • Keywords has-patch added; needs-patch removed

I figured that a "settings-field" class isn't necessary, since you can target fields using .form-table tr

Last edited 13 years ago by scribu (previous) (diff)

#11 @nacin
13 years ago

They may not remain tables for long.

@scribu
13 years ago

Added settings-field class

#12 @scribu
13 years ago

Good point; updated patch.

#13 @scribu
13 years ago

  • Keywords 3.3-early removed
  • Milestone changed from Future Release to 3.3

If we make no progress on the settings api at all, we can at least do this and it will be a nice step forward.

#14 @griffinjt
13 years ago

Agreed. Simple and can let us have a little more control over appearances. :)

#15 @ryan
13 years ago

  • Milestone changed from 3.3 to Awaiting Review

Punting enhancements from 3.3.

#16 @ryan
13 years ago

  • Milestone changed from Awaiting Review to Future Release

#17 @chriscct7
10 years ago

  • Keywords dev-feedback added

#18 @DrewAPicture
10 years ago

  • Keywords needs-refresh added

#19 @chriscct7
9 years ago

  • Keywords good-first-bug added; dev-feedback removed

@ross_ritchey
9 years ago

Add new $args to add_settings_section. Update add_settings_section to include new $args in array. Add wrapper around printout in do_settings_section.

#20 @ross_ritchey
9 years ago

Just put a patch in. First-time patcher - not positive everything was done kosher.

Looking at the history - this looks like something that could be a good little enhancement. I kept backwards-compatibility while also pushing towards more logical handling. Not sure if there is a way to degrade the current add_settings_section arguments to a single $args array without breaking compatibility - but could be nice if there was.

#21 @chriscct7
9 years ago

  • Owner set to chriscct7
  • Status changed from reopened to reviewing

@ross_ritchey
8 years ago

Refresh of patch

#22 @ross_ritchey
8 years ago

  • Keywords needs-unit-tests needs-testing added; needs-refresh removed

Swinging back around to this patch and it looks like it needed a refresh as template.php changed a little in the intervening year.

This request still seems relevant -> just updated the patch to match the current template.php

@palmiak
6 years ago

Refreshed template.php file

@rehanali
3 years ago

Added patch

This ticket was mentioned in Slack in #core by sergey. View the logs.


2 years ago

#24 @SergeyBiryukov
2 years ago

  • Milestone set to 6.1

@costdev
2 years ago

Patch #17851.patch refreshed against trunk, updated with coding standards, docblock updates, and the new, but unused, $section variable is now used.

@martin.krcho
2 years ago

This is updated patch 17851.4.diff with added unit tests.

#25 @martin.krcho
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests needs-testing removed

This ticket was mentioned in PR #3099 on WordPress/wordpress-develop by martinkrcho.


2 years ago
#26

This PR improves the add_settings_section function to allow developers to pass extra HTML mark-up to be rendered before and after the settings section. Extra argument $args can now be passed to the function. It's an array that can contain the following items:

  • before_section
  • after_section
  • section_class

Trac ticket: https://core.trac.wordpress.org/ticket/17851

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#28 @audrasjb
2 years ago

  • Keywords changes-requested needs-dev-note needs-testing added

As per today's bug scrub, @costdev mentioned:

Multiple-assertion test methods in the PR will need a $message parameter added to each assertion (Ref) -> "This parameter should always be used if more than one assertion is used in a test method."

Let's update the provided unit tests.
Self assigning for proper testing.

#29 @audrasjb
2 years ago

  • Owner changed from chriscct7 to audrasjb

#30 @martin.krcho
2 years ago

  • Keywords has-dev-note added; changes-requested needs-dev-note removed

I added the messages to each of the new assertions in commit df015d79b8120b1f0312568b41ce3dd0e1d62667. You can go ahead with testing the ticket properly, @audrasjb :)

This ticket was mentioned in Slack in #core by martin.krcho. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


2 years ago

#33 @chaion07
2 years ago

Thanks @griffinjt for reporting this. We reviewed this ticket during a recent bug-scurb session. Based on the feedback received from the team here's a summary:

  1. The PR is ready for review
  2. We need @audrasjb feedback on the PR and possibly a follow up message
  3. Need more eyes on this

Cheers!

Props to @mukesh27 and @martinkrcho

#34 follow-up: @audrasjb
2 years ago

  • Keywords needs-dev-note commit added; needs-testing has-dev-note removed
  • Status changed from reviewing to accepted

Looks good to go on my side.
I was able to add extra HTML content before and after the settings section I created, and a class using the %s placeholder.

Let's go with this implementation, thanks everyone.

PS: @martinkrcho please don't remove the needs-dev-note keyword since it indicates that we need a developer note on make.wordpress.org/core for this change :)

#35 @audrasjb
2 years ago

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

In 54247:

Administration: Allow to wrap Settings sections with custom HTML content.

This changeset improves the add_settings_section() function to allow developers to pass extra HTML mark-up to be rendered before and after the settings section. Extra argument $args can now be passed to the function, and is an array that can contain the following items:

  • before_section: HTML content to prepend to the section's HTML output. Receives the section's class name provided with the section_class argument via an optional %s placeholder. Default empty.
  • after_section: HTML content to append to the section's HTML output. Default empty.
  • section_class: The class name to use for the section. Used by before_section if a %s placeholder is present. Default empty.

The HTML passed using these extra arguments is escaped using wp_kses_post() just before rendering. This changeset also provides a set of unit tests for this new feature.

Props griffinjt, nacin, scribu, ross_ritchey, ryan, chriscct7, palmiak, rehanali, costdev, martinkrcho, chaion07, audrasjb, hellofromtonya.
Fixes #17851.

#37 in reply to: ↑ 34 @martin.krcho
2 years ago

Replying to audrasjb:

PS: @martinkrcho please don't remove the needs-dev-note keyword since it indicates that we need a developer note on make.wordpress.org/core for this change :)

I'll keep it in mind for next time. Thank you for committing the changes.

#38 @peterwilsoncc
2 years ago

@mukesh27 added to props via make/core per comment #33.

Note: See TracTickets for help on using tickets.