Bug 8453 - need spaces after radio buttons on inventory
Summary: need spaces after radio buttons on inventory
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low trivial (vote)
Assignee: Nicole C. Engard
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-16 16:17 UTC by Nicole C. Engard
Modified: 2013-12-05 20:05 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8453: Add spaces to inventory options (1.48 KB, patch)
2012-07-16 16:19 UTC, Nicole C. Engard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8453: Add spaces to inventory options (1.58 KB, patch)
2012-07-17 12:33 UTC, Owen Leonard
Details | Diff | Splinter Review
Another proposed patch (1.53 KB, patch)
2012-07-17 15:27 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2012-07-16 16:17:55 UTC
On the inventory screen the library radio buttons are smooched against their labels.  Patch forthcoming.
Comment 1 Nicole C. Engard 2012-07-16 16:19:13 UTC Comment hidden (obsolete)
Comment 2 David Cook 2012-07-16 23:21:52 UTC
(In reply to comment #1)
> Created attachment 10876 [details] [review]
> Bug 8453: Add spaces to inventory options
> 
> The home and current library labels were right on top
> of the radio buttons. This patch just adds a bit of
> space in there so that it's cleaner looking.

While I'm not sure of the standard Koha practice, I think that it would be a better idea to use CSS to control this rather than putting in a hard-coded space. From what I can remember, I think best practice is to always use CSS to control display and to avoid hard-coded spaces wherever possible.

We could probably do something along the lines of #tools_inventory * input[type="radio"][name="branch"] {margin-right:5px;} or insert an ID or two and select those, if we don't want to rely on attribute selectors.
Comment 3 Nicole C. Engard 2012-07-17 01:26:43 UTC
I'm pretty sure that all other places this is just a space. I agree that CSS makes the most sense if you want to add in consistent spacing, I'm just making a space after the radio button and before the text ... very standard kind of thing that I think CSS would be overkill for.
Comment 4 David Cook 2012-07-17 02:21:39 UTC
(In reply to comment #3)
> I'm pretty sure that all other places this is just a space. I agree that CSS
> makes the most sense if you want to add in consistent spacing, I'm just
> making a space after the radio button and before the text ... very standard
> kind of thing that I think CSS would be overkill for.

If all the other places are using a space, that space should probably be removed and then CSS used to apply that style across the board, since we are trying to provide consistent spacing between the radio button and the text. After all, what happens when we decide that we want no space or a wider space? The downfall of using the hard-coded space is the width of your space is hard-coded too. No matter how many spaces are added to the HTML, only that one space will appear on the page. I think CSS provides the most flexibility and is a better practice in general.

While I'm having trouble finding an authoritative source to link to that says not to use spaces for display purposes in HTML, I've been told this in classes and by other programmers as well. Admittedly, it's a fairly trivial localized display issue, but I think that's how widespread inconsistencies start.
Comment 5 Owen Leonard 2012-07-17 12:30:21 UTC
(In reply to comment #4)
> If all the other places are using a space, that space should probably be
> removed and then CSS used to apply that style across the board

It is a consistent practice across the board to use a single space around inputs and labels, just as you would include a single space after the end of a sentence. I would consider this to be "minimum safe distance" for readability. If you want to get into adding *more* space around elements, that's when you start to look at adding CSS.
Comment 6 Owen Leonard 2012-07-17 12:33:14 UTC
Created attachment 10889 [details] [review]
[SIGNED-OFF] Bug 8453: Add spaces to inventory options

The home and current library labels were right on top
of the radio buttons. This patch just adds a bit of
space in there so that it's cleaner looking.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 7 Jonathan Druart 2012-07-17 15:27:15 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2012-07-17 15:40:33 UTC
(In reply to comment #7)
> I agree with David, I think it is a good practice to use CSS to control
> ajustment.

Yes, but reducing the level of granularity to that of a single space is taking this good practice too far.
 
> But in this case, it is possible to do more sexy radio buttons using labels.

Yes, your patch is a more thorough fix, giving us clickable labels. However, it still relies on whitespace to preserve spacing around the elements ;)
Comment 9 Nicole C. Engard 2012-07-17 15:48:35 UTC
And putting this in to discussion is just delaying an easy fix from making it in to Koha.  

I say leave my tini tiny itty bitty harmless patch as it is and if you want to follow up with a more thorough patch go for it.  

Some of us (like me) don't have a lot of things to contribute to the code and so when I make string and spacing fixes and they get trashed cause they're not complicated enough it's kind of discouraging and the reason why more people without lots of programming skills aren't submitting patches.
Comment 10 David Cook 2012-07-17 23:31:34 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > I agree with David, I think it is a good practice to use CSS to control
> > ajustment.
> 
> Yes, but reducing the level of granularity to that of a single space is
> taking this good practice too far.
>  
> > But in this case, it is possible to do more sexy radio buttons using labels.
> 
> Yes, your patch is a more thorough fix, giving us clickable labels. However,
> it still relies on whitespace to preserve spacing around the elements ;)

After a quick look around at subscriptions-add.pl and import_borrowers.pl, it seems that clickable labels (rather than a single space) are already the standard practice and inventory.pl was just missed.

There is already an existing "radio" class for the "li" element that adds the whitespace between the radio button and the clickable label (which would also need to be introduced). 

I know this is a trivial issue, but I'm still on the side of consistency.
Comment 11 Chris Cormack 2012-07-17 23:34:27 UTC
(In reply to comment #10)

> There is already an existing "radio" class for the "li" element that adds
> the whitespace between the radio button and the clickable label (which would
> also need to be introduced). 
> 
> I know this is a trivial issue, but I'm still on the side of consistency.

I think you and Owen might be talking past each other. Adding the space makes the html more readable, using css makes the rendered page more readable. Both have merit.
Comment 12 David Cook 2012-07-18 00:10:52 UTC
(In reply to comment #9)
> And putting this in to discussion is just delaying an easy fix from making
> it in to Koha.  
> 
> I say leave my tini tiny itty bitty harmless patch as it is and if you want
> to follow up with a more thorough patch go for it.  
> 
> Some of us (like me) don't have a lot of things to contribute to the code
> and so when I make string and spacing fixes and they get trashed cause
> they're not complicated enough it's kind of discouraging and the reason why
> more people without lots of programming skills aren't submitting patches.

Nicole, I still maintain that an easy fix isn't always the right fix. The more I work with Koha, the more I see a need for consistent practice. Paul failed my Bug 7839 fix, because I didn't adhere to the spelling and naming guidelines. While that might seem trivial, I see the logic in preserving consistency. I was happy to re-write my patch. 

However, I do apologize for making it seem like I was trashing your fix. That was not at all my intention. I think that a lot of the fixes you do, especially in regards to documentation, are absolutely invaluable. In the future, I will try to be more positive in my feedback. I don't think patches need to be complex to be useful.

In any case, I've thrown in my two cents, so I'll bite my tongue now :p.
Comment 13 Jonathan Druart 2012-07-18 08:12:45 UTC
Hi Nicole and Owen,

I agree, It is a lot of discussion for a little patch.
But I think this kind of patch should have never exist :)

Let me explain you why.

When I arrived on this patch to make QA, I thought: it is an easy patch. I was surprised to find an unique space for a patch. I didn't know we wanted to add space after a input radio.

Apparently, I am not alone in this case.
My job as a part of QA team is to put in question each patch. I think this one should not have exist, because the ajustement between input and label should automatically be taken into account.
Otherwise, for each patch adding these lines, inevitably a patch will have to be created to fix that. It's not very productive, I think.
Given that, we can fix that definitely and don't create a new patch for each space to add.

You patch works well, but it fixes one problem rather than fixing all futur problems

Marking as Passed QA
Comment 14 Paul Poulain 2012-07-18 13:18:53 UTC
(In reply to comment #13)
> Hi Nicole and Owen,
> 
> I agree, It is a lot of discussion for a little patch.
> But I think this kind of patch should have never exist :)
> 
> Let me explain you why.
> 
> When I arrived on this patch to make QA, I thought: it is an easy patch. I
> was surprised to find an unique space for a patch. I didn't know we wanted
> to add space after a input radio.
> 
> Apparently, I am not alone in this case.
> My job as a part of QA team is to put in question each patch. I think this
> one should not have exist, because the ajustement between input and label
> should automatically be taken into account.
> Otherwise, for each patch adding these lines, inevitably a patch will have
> to be created to fix that. It's not very productive, I think.
> Given that, we can fix that definitely and don't create a new patch for each
> space to add.
> 
> You patch works well, but it fixes one problem rather than fixing all futur
> problems
> 
> Marking as Passed QA

We're not really consistent:
 * moremember.pl page (patron adding) show yes [] no [] for flags & sex (checkbox on the right of the label)
 * the inventory.pl page show the checkbox on the left, description is not clicable
 * the tools/holidays.pl page show the checkbox on the left, the description itself is clickable

My preference goes to what we have in tools/holidays.pl, but let me know your's.

Should we start a discussion on koha-devel for that ?

In the meantime, this patch is pushed, even if i'm not a fan of the space (owen, your voice is a leader one for all what is related to interface for me)
Comment 15 Chris Cormack 2013-04-20 11:02:20 UTC
Released in 3.10.0