At admin/settings/statistics a number of radios are shown to enable or disable certain logs. These should be replaced by checkboxes.

CommentFileSizeAuthor
#22 Statistics-before.png38.59 KBmrfelton
#22 Statistics-after.png33.17 KBmrfelton
#19 362852-statistics-admin-form-cleanup-D7.patch2.92 KBdave reid
#8 362852-statistics-admin-form-cleanup-D7.patch2.92 KBdave reid
#7 362852-statistics-admin-form-cleanup-D7.patch2.67 KBdave reid

Comments

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

I might have explained my reasons for submitting this issue a bit more extensively:

  1. The settings this issue addresses are about enabling a certain feature, saying yes or no to something. Currently, the radios' labels do not add any information that hasn't already been given. Also, according to Jakob Nielsen A stand-alone checkbox is used for a single option that the user can turn on or off. (source)
  2. It takes up less space on the page.
  3. It requires less code.
xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Assigned: Unassigned » xano

Just figured "Why not?"

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

I'm agreed these options will probably cleaner as checkboxes.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Assigned: xano » Unassigned

Okaaaaay, wrong issue (A). Do intend to work on it, just haven't started yet.

damien tournoud’s picture

damien tournoud commented

There is not need to duplicate the component attribute.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

What do you mean?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Title: Enabled/Disabled radios should be checkboxes » Clean up statistics admin settings
Status: Active » Needs review
StatusFileSize
new 362852-statistics-admin-form-cleanup-D7.patch2.67 KB
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 362852-statistics-admin-form-cleanup-D7.patch2.92 KB

Fixed closing array indentation.

Status: Needs review » Needs work

The last submitted patch failed testing.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review

I don't believe you testing bot! :)

Status: Needs review » Needs work

The last submitted patch failed testing.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review

Dear Testing Bot,

I really don't believe you. This patch has nothing to do with the 'Field form tests.'

Regards,
Dave

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

I guess you did it. The bar looks bright green over here :D

Bojhan’s picture

Bojhan commented

Do you have a screenshot?

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

Clicketyclick.

cburschka’s picture

cburschka
they
commented
Status: Needs review » Reviewed & tested by the community

Very nice code-style and comment fix along with those checkboxes.

I see nothing that could be improved in this patch, and it seems Testbot finally likes it as well.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs screenshots

Heh, #14 was asking for a before/after screenshot of the feature, not testing bot. ;) Let's add this, and additionally:

+  // Access log settings

Comments start with a capital letter, end with a period. This applies throughout the patch.

   $form['access'] = array(
     '#type' => 'fieldset',
+    '#title' => t('Access log settings')
+  );

The final index in an array should end with a comma as well -- so t('Access log settings'),). It looks like a mistake but isn't. This applies throughout the patch.

cburschka’s picture

cburschka
they
commented

+ '#title' => t('Access log settings')

Ack, I need new glasses! =D

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review
StatusFileSize
new 362852-statistics-admin-form-cleanup-D7.patch2.92 KB

Revsied patch. Thanks for catching those, not sure why I missed those! *slaps face*

Stefan Nagtegaal’s picture

Stefan Nagtegaal commented
Status: Needs review » Reviewed & tested by the community

Nice one!
tested and verified to work on my D7 install.. Pretty RTBC.. Lets commit this webchick! :-)

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Still needs before/after screenshots.

mrfelton’s picture

mrfelton commented
StatusFileSize
new Statistics-after.png33.17 KB
new Statistics-before.png38.59 KB

Here is a before and after screenshot. This is the correct way to do it for simple on/off settings like this. +1

Bojhan’s picture

Bojhan commented
Status: Needs review » Reviewed & tested by the community

Looks good, marking RTBC

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented

Can't we get rid of the fieldets as well?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Now that's much better. Committed to HEAD. Thanks! :)

Stefan Nagtegaal’s picture

Stefan Nagtegaal commented

We do this in *a lot* of UI-places in drupal. For example, the whole Create node type thing is full of the "wrong" usage of UI-elements.
I am pretty sure, I filed a bug report/feature request for this using proper forms years ago.

Unfortunatly I can not find it anymore...

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

This is the first committed core patch without failures I am looking at today. Thank you!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.