About issue 1951

[Jesús García Crespo]

Hi Wu,

David let me know that you sent us a new patch for issue 1951. Sorry for any inconveniences, I did archived your this issue change notification accidentally in my webmail.

Thank you so much! I'll review your code as soon as possible.

Regards,

--
Jesús García Crespo

[Jesús García Crespo]

Hi Wu,

I have just seen your patch and there are some points that we should discuss:

1) Please, remember that your patches should contain only relative paths. A nice tool to create new patches is "svn diff", but it needs a local copy of our SVN repository (trunk or 1.1 branch). Otherwise, we'd need to adapt your patches to be applicable.

2) Remeber our coding standards. We don't use tabs to indent, instead, we use two whitespaces. You can setup your text editor to use whitespaces in any case.

3) I see that you put the pager at the top of the table as well as we do at the bottom. This is not a bad idea at all, but I think that is a big change that we should do everywhere, in all our pages at the same time for unity in our application. If you really want to do this, you can create a new issue to track it.

4) With this new fourth column, the table gets very crowded (see attached screenshot, table.png). It is readable but we may need to do some improvements on this point. What do you think? An alternative would be to show the Archival institution value after the title, sharing the same column.

5) I see that you removed some lines of code like those which get app_multi_repository and other config attributes (original line 32), it was intentional? That produces some empty PHP conditions like these between lines 32 and 33 in your patch.

I am being very meticulous but I hope that you understand that is important to keep the quality of the project. In the other hand, I think that you doing big progress!

Regards,

--
Jesús García Crespo,
Software Engineer, Artefactual Systems Inc.
http://www.artefactual.com | +1.604.527.2056

[David Juhasz]

On 11-03-08 03:13 PM, Jesús García Crespo wrote:

Hi Wu,

I have just seen your patch and there are some points that we should discuss:

1) Please, remember that your patches should contain only relative paths. A nice tool to create new patches is "svn diff", but it needs a local copy of our SVN repository (trunk or 1.1 branch). Otherwise, we'd need to adapt your patches to be applicable.

2) Remeber our coding standards. We don't use tabs to indent, instead, we use two whitespaces. You can setup your text editor to use whitespaces in any case.

More specifically we use two "space" characters for indenting.

3) I see that you put the pager at the top of the table as well as we do at the bottom. This is not a bad idea at all, but I think that is a big change that we should do everywhere, in all our pages at the same time for unity in our application. If you really want to do this, you can create a new issue to track it.

Adding a pager to the top of the page was one of the changes in the original SCAA feature list, but we felt that it would be better to evaluate this feature based on our usability testing.   At this point if you decide to implement a pager at the top of the page it would be a change specific to the the U. Sask site.

4) With this new fourth column, the table gets very crowded (see attached screenshot, table.png). It is readable but we may need to do some improvements on this point. What do you think? An alternative would be to show the Archival institution value after the title, sharing the same column.

Sorry, I didn't anticipate this problem when Tim asked me about it on the phone.   One other option I can think of for freeing up horizontal space is to remove the "level" column.

5) I see that you removed some lines of code like those which get app_multi_repository and other config attributes (original line 32), it was intentional? That produces some empty PHP conditions like these between lines 32 and 33 in your patch.

I am being very meticulous but I hope that you understand that is important to keep the quality of the project. In the other hand, I think that you doing big progress!

Thanks for your contributions Wu!

-- 
David Juhasz,
Software Engineer

Artefactual Systems Inc.
www.artefactual.com 

[Peter Van Garderen]

>>
>> 4) With this new fourth column, the table gets very crowded (see attached
>> screenshot, table.png). It is readable but we may need to do some improvements
>> on this point. What do you think? An alternative would be to show the Archival
>> institution value after the title, sharing the same column.

Keep in mind that this is using a fixed-with table in a fixed-width theme. So
this could also be addressed by making better use of horizontal space and making
the SCAA theme full/relative width.

--peter

[Tim Hutchinson]

> 5) I see that you removed some lines of code like those which get
> app_multi_repository and other config attributes (original line 32),
> it was intentional? That produces some empty PHP conditions like these
> between lines 32 and 33 in your patch.

Another side effect of this (I hadn't tested this before) is that the
institution column shows up in both views, even if multi-repository
isn't selected.

> 4) With this new fourth column, the table gets very crowded (see
> attached screenshot, table.png). It is readable but we may need to do
> some improvements on this point. What do you think? An alternative
> would be to show the Archival institution value after the title,
> sharing the same column.
>
> Sorry, I didn't anticipate this problem when Tim asked me about it on
> the phone. One other option I can think of for freeing up horizontal
> space is to remove the "level" column.

That sounds OK to me - that would similar to the approach taken for the
alphabetical view - there is a creator column in place of repository if
multi-repository isn't selected.

Alternatively, Peter mentioned the theme development. If we need to
pursue this as part of theme development, that's fine. This was supposed
to be an easy patch but I guess there are side effects we didn't think
of. And a site-wide change (e.g. 4th column) obviously shouldn't rely on
which theme is being used.

Tim

--
Tim Hutchinson
University of Saskatchewan Archives
301 Main Library, 3 Campus Drive
Saskatoon, SK S7N 5A4
tel: (306) 966-6028
fax: (306) 966-6040
e-mail: tim.hut...@usask.ca
web: http://www.usask.ca/archives/

[Tim Hutchinson]

To follow up on this further, as a core contribution I would suggest the
following:

- leave alphabetical view as is
- for recent changes view, replace Level column with Institution column
when multi-repository is set to true (no change otherwise). This would
be consistent with the approach taken with the alphabetical view
(omitting one of the columns available in single repository mode).

If this is acceptable, we'll proceed on that basis and Wu will submit a
revised patch. Otherwise, for example if you want to wait for more
usability testing, we will incorporate this into theme development or
use a customization. Please let us know.

Thanks
Tim

[peterVG]

> Alternatively, Peter mentioned the theme development. If we need to
> pursue this as part of theme development, that's fine. This was supposed
> to be an easy patch but I guess there are side effects we didn't think
> of. And a site-wide change (e.g. 4th column) obviously shouldn't rely on
> which theme is being used.

Yes, if this is going to be submitted as a core contribution then I
guess it should work with the default fixed width theme. I would like
us to switch to a relative width theme as our default in a future
release. That will give us more flexibility in dealing with issues
like this (although it may make maintaining fixed width themes that
much more difficult).

--peter

[peterVG]

> - leave alphabetical view as is
> - for recent changes view, replace Level column with Institution column
> when multi-repository is set to true (no change otherwise). This would
> be consistent with the approach taken with the alphabetical view
> (omitting one of the columns available in single repository mode).
>

Yes, I think this is the best solution, let's proceed with this.

--peter

[Tim Hutchinson]

Great, thanks.

Tim

[wu....@usask.ca]

I just created a new patch and its for both issue 1932 and 1951.
please see here:http://code.google.com/p/qubit-toolkit/issues/detail?
id=1951

> e-mail: tim.hutchin...@usask.ca
> web:http://www.usask.ca/archives/