Re: [Spacewalk-devel] [PATCH] Date/Time picker

2014-02-18 Thread Michael Mraka 
Duncan Mac-Vicar P. wrote:
% Thanks for the review and fixes!. Most of it looks good except for the
% self-closing tag as Silvio explained.
% 
% There are other changes that I would like to understand:
% 
% - The css part where you override some styles (and use !important, which
% basically means SUSE Manager can't override it again in its css)

They override stuff from datepicker/timepicker css. Because css files
order they have to be forced with !important.

% - setting of the input size to 15

To set length ratio of date and time input boxes. Otherwise they are
equally long. Is there a better way to enforce it?
 

Regards,

--
Michael Mráka
Satellite Engineering, Red Hat

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Re: [Spacewalk-devel] [PATCH] Date/Time picker

2014-02-17 Thread Matej Kollar 
  On 02/14/2014 03:16 PM, Michael Mraka wrote:
should likely be / otherwise it produce opening tags not self
   closed tags.
  
  Actually in HTML5 void tags do not need the slash (it is optional).
  
  http://www.w3.org/TR/html-markup/syntax.html#void-element
  
  self-closing as a definition does not exist anymore in HTML5, except
  for SVG and MathML elements!
  
  We also recently started a discussion about this same topic and decided
  to stick to a coding standard rule to avoid confusion - namely not using
  those optional slashes in new code.
  
  I hope you are also fine with that. I was supposed to start a discussion
  with you in the next days, but this was faster :-)
 
 I am strongly against it.

Ok, let me go a little deeper than that:

  * standard allows closing of tags,
  * HTML is generated on some places explicitly... are you sure
it would not lead to state where we would be inconsistent (=bad),
  * in case we switch (and do it properly and completely) it would
make it much harder to switch to say XHTML if desired,
  * without closing tags it would be like
those evil old times with wild HTML...
(not speaking about omitting closing tags...)
  * code that Michael commented on was wrong anyway as it only
deleted symbol and immediately pended it.

Matej

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Re: [Spacewalk-devel] [PATCH] Date/Time picker

2014-02-15 Thread Matej Kollar 


--
Matej Kollar
Satellite Engineering, Red Hat

- Original Message -
 From: Silvio Moioli smoi...@suse.de
 To: spacewalk-devel@redhat.com
 Sent: Friday, February 14, 2014 3:43:32 PM
 Subject: Re: [Spacewalk-devel] [PATCH] Date/Time picker
 
 On 02/14/2014 03:16 PM, Michael Mraka wrote:
   should likely be / otherwise it produce opening tags not self
  closed tags.
 
 Actually in HTML5 void tags do not need the slash (it is optional).
 
 http://www.w3.org/TR/html-markup/syntax.html#void-element
 
 self-closing as a definition does not exist anymore in HTML5, except
 for SVG and MathML elements!
 
 We also recently started a discussion about this same topic and decided
 to stick to a coding standard rule to avoid confusion - namely not using
 those optional slashes in new code.
 
 I hope you are also fine with that. I was supposed to start a discussion
 with you in the next days, but this was faster :-)

I am strongly against it.

 Thanks,
 --
 Silvio Moioli
 SUSE LINUX Products GmbH
 Maxfeldstraße 5, 90409 Nürnberg Germany
 
 ___
 Spacewalk-devel mailing list
 Spacewalk-devel@redhat.com
 https://www.redhat.com/mailman/listinfo/spacewalk-devel

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Re: [Spacewalk-devel] [PATCH] Date/Time picker

2014-02-14 Thread Michael Mraka 
Duncan Mac-Vicar P. wrote:
% See attached patches.
% 
% They fix
% 
% - time localization
% - first day of the week
% - clicking on time picker
% - autoclose
% - move the libraries to separate package

Hello Duncan,

I've reviewed and committed date picker patches. Although I have some
objections I've decided to fix it rather then send it back and forth.


...
% +private String toPhpTimeFormat(String format) {
% +String ret = format.replaceAll((a)+, a);
% +ret = ret.replaceAll((H)\\1+, H);
% +ret = ret.replaceAll((H), G);

This is wrong. It will replace HH with H and immediately H with G which
is most likely not what you meant.

% +// k (0-24) not supported, convert to the 0-23 format
% +ret = ret.replaceAll((k)\\1+, H);
% +ret = ret.replaceAll((k), G);
% +// K (0-11) not supported, convert to the 1-12 format
% +ret = ret.replaceAll((k)\\1+, h);
% +ret = ret.replaceAll((k), g);

This should likely be (K) not (k).

% +ret = ret.replaceAll((h)\\1+, h);
% +ret = ret.replaceAll((h), g);

Again hh is replaced with h and immediately h with g.

% +ret = ret.replaceAll((m)+, i);
% +ret = ret.replaceAll((s)+, s);
...
% @@ -159,15 +192,18 @@ public class DateTimePickerTag extends TagSupport {
%  
%  HtmlTag dateAddon = createInputAddonTag(date, fa fa-calendar);

For icons there's IconTag which ensures consistent usage of icons.
(So that one won't use fa-calendar while others use fa-calendar-o.)

...
% +/**
% + * Convert day java.util.Calendar constants
% + * (for which we can't assume a fixed value)
% + * to an index usable by the javascript picker.
% + *
% + * @return the equivalent index for the javascript
% + * picker
% + */
% +private int getJavascriptPickerDayIndex(int calIndex) throws 
IllegalArgumentException {
% +switch (calIndex) {
% +case Calendar.SUNDAY:return 0;
% +case Calendar.MONDAY:return 1;
% +case Calendar.TUESDAY:   return 2;
% +case Calendar.WEDNESDAY: return 3;
% +case Calendar.THURSDAY:  return 4;
% +case Calendar.FRIDAY:return 5;
% +case Calendar.SATURDAY:  return 6;
% +default: throw new IllegalArgumentException(Invalid day  + 
calIndex);
% +}
% +}

I think such switch statement is really overkill. Yes, in theory
calendar constant values may change in the future but in fact they are pretty
solid for couple of years now. 

...
% +private String toWeirdDateFormat(String format) {
% +String ret = format.replaceAll((M)\\1\\1\\1+, MM);
% +ret = ret.replaceAll(MMM, M);
% +ret = ret.replaceAll(MM, mm);
% +ret = ret.replaceAll(M, m);

Exactly the same problem I described in toPhpTimeFormat().
 is replaced by MM which is then replaced by mm.
MMM is replaced by M which is then replaced by m.

% +ret = ret.replaceAll((E)\\1\\1\\1+, DD);
% +ret = ret.replaceAll(E+, D);
% +ret = ret.replaceAll((D)\\1+, dd);
% +ret = ret.replaceAll(D, d);

And again  to DD to dd.

% +ret = ret.replaceAll((y)\\1\\1\\1+, );
% +ret = ret.replaceAll(y+, yy);
% +return ret;
% +}

And again.

% +private void writePickerHtml(Writer out) throws IOException {
...
% +HtmlTag timeAddon = createInputAddonTag(time, fa fa-clock-o);
% +
% +HtmlTag timeInput = new HtmlTag(input);
% +//dateInput.setAttribute(value, data.getDate().toString());
% +timeInput.setAttribute(type, text);
% +timeInput.setAttribute(class, form-control);
% +timeInput.setAttribute(id, data.getName() + 
_timepicker_widget_input);
% +
% +HtmlTag tzAddon = new HtmlTag(span);
% +tzAddon.setAttribute(id, data.getName() + _tz_input_addon);
% +tzAddon.setAttribute(class, input-group-addon);
% +tzAddon.addBody(
% +data.getCalendar().getTimeZone().getDisplayName(
% +false, TimeZone.SHORT, data.getLocale()));
% +
% +HtmlTag col2 = new HtmlTag(div);
% +col2.setAttribute(class, col-md-5);
% +
% +group.addBody(dateAddon);
% +group.addBody(dateInput);
% +if (!data.getDisableTime()) {
% +group.addBody(timeAddon);
% +group.addBody(timeInput);
% +group.addBody(tzAddon);
% +}

Whole timeAddon and timeInput should be better placed inside the if block.
So they aren't even generated when they aren't used later.

...
% +private void writeI18NMap(Writer out) throws IOException {
% +// generate i18n for the picker here
% +DateFormatSymbols syms = data.getDateFormatSymbols();
% +out.append(script type='text/javascript'\n);
% +out.append($.fn.datepicker.dates[' + data.getLocale() + '] = 
{\n);
% +out.append(days: [ \n);
% +out.append(String.format(  '%s',

Re: [Spacewalk-devel] [PATCH] Date/Time picker

2014-02-14 Thread Silvio Moioli 
On 02/14/2014 03:16 PM, Michael Mraka wrote:
  should likely be / otherwise it produce opening tags not self
 closed tags.

Actually in HTML5 void tags do not need the slash (it is optional).

http://www.w3.org/TR/html-markup/syntax.html#void-element

self-closing as a definition does not exist anymore in HTML5, except
for SVG and MathML elements!

We also recently started a discussion about this same topic and decided
to stick to a coding standard rule to avoid confusion - namely not using
those optional slashes in new code.

I hope you are also fine with that. I was supposed to start a discussion
with you in the next days, but this was faster :-)

Thanks,
-- 
Silvio Moioli
SUSE LINUX Products GmbH
Maxfeldstraße 5, 90409 Nürnberg Germany

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Re: [Spacewalk-devel] [PATCH] Date/Time picker

2014-02-14 Thread Duncan Mac-Vicar P. 

Thanks for the review and fixes!. Most of it looks good except for the
self-closing tag as Silvio explained.

There are other changes that I would like to understand:

- The css part where you override some styles (and use !important, which
basically means SUSE Manager can't override it again in its css)
- setting of the input size to 15

-- 
Duncan Mac-Vicar P. - http://www.suse.com/

SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix
Imendörffer, HRB 16746 (AG Nürnberg)
Maxfeldstraße 5, 90409 Nürnberg, Germany

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Re: [Spacewalk-devel] [PATCH] Date/Time picker

2014-02-10 Thread Silvio Moioli 
On 02/06/2014 11:12 AM, Duncan Mac-Vicar P. wrote:
 See attached patches. [...]

On a slightly unrelated note, I have just noticed the current date
picker allows incorrect dates to be picked, such as February 31. In the
SSM errata scheduling page, among other things, such values cause a NPE.

Regards,
-- 
Silvio Moioli
SUSE LINUX Products GmbH
Maxfeldstraße 5, 90409 Nürnberg Germany

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Re: [Spacewalk-devel] [PATCH] Date/Time picker

2014-02-06 Thread Michael Mraka 
Duncan Mac-Vicar P. wrote:
%  Usability glitches:
%  - date changes correctly accroding to localization but time is always
%12 hour a.m./p.m. in any localization
% I left this because it was like that in the current date picker. I will
% improve this, but I have to first make sure the date picker object (Java
% part) support this (if needed at all).

Well, either don't bother with localization at all or make it complete.
Half done things are annoying.

%  - week starts on Sunday in all localizations
% Fixed, I will include it in an updated patch.

Thanks.

%  - clicking on calendar icon pops-up date picker but clicking on clock
%icon doesn't pop-up time picker
% Fixed by updating the library

Thanks.

%  - calendar pops above picker, time pulls down
% We could enforce this, but in general the direction is calculated on
% available space, I guess this happens because the picker is at the end
% of the page. Enforcing may have some nasty side-effects

Nope, there's plenty of space both above and bellow the picker. IMHO 
auto-bottom option opens it bellow the picker by default.

%  - if I click for the first time to date calendar doesn't highlight
%currently selected date (it highlights always today)
% The start date is today, and the calendar highlights today and the
% selected date in different colors.
% So I am not sure I understand what is the problem here.

E.g in monitoring probe there's picker which is set some time in the
future (or in the past). When I click it shows calendar with highlighted
today not the date from the picker.

%  - calendar should close when I click chosen date (or at least at
%doubleclick)
% Fixed, will go in next patch.

Thanks.

%  Implementation:
%  - What are fn.datepicker.dates arrays needed for? It looks like a kind
%of localization but AFAIK bootstrap datepicker cares about
%localization itself (language option).
% That way instead of using the built-in locale files from javascript I
% generate
% the localization from the Java strings (Calendar). That way we don't
% have to include the localization .js files and to track them for
% translation.

Well, I'd prefer including static javascript to generating it over and
over with every picker. Even those static localization js can be
generated automatically in rpm build time (from the same sources you
generate it now). Anyway I'm not pushing for it.

%  - bootstrap-datepicker / jquery.timepicker css and js should not be in
%branding or web but in a separate package which is required by branding.
%  - What's the benefit of combining bootstrap datepicker with jquery
%timepicker over some bootstrap datetimepicker ([1], [2]) which does
%both?
% 
% We tried different combinations of pickers. We all kind of agreed that
% the combination of these two give you something very similar to the
% Facebook picker which is a very good balance between easy and convenient
% with the keyboard (you can still use it) from all the ones we tried.
% Cynthia, our UX developer also agreed. So I would say we optimized more
% the user experience than the packaging or the code.

I agree with user experience on the other hand this combination, for me,
isn't good experience. They are two different libs / different styles.
There's visible difference between date picker - pop up window - and
time picker - classic pull down menu.

% bootstrap-datetimepicker is also an external library, combining two
% pickers in one, but the date picker is not very nice. You can try it
% yourself and you will see what I am talking about :-), also I suspect
% Facebook also experimented a lot with their picker already. The second
% one you linked is a bit better, though it forces you to go through the 3
% steps.

I've tried [1]. I didn't liked it at the beginning but
after some time I found it in fact quite good. You can set the time with
a precision of seconds in just 3 clicks (try clicking on second number).
Yes, it's a bit uglier on the other hand both date and time part has consistent
look and feel.

The [2] is also more consistent, looks IMHO a bit better then [1], and
allows to set time with 5 min precision.


% Duncan

Regards,

--
Michael Mráka
Satellite Engineering, Red Hat


___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel