= Review (NS-79): FR 1511, added localization for numbers in webpages. Almost... =
|| Author || Henrik Kirk ||
|| Moderator || Henrik Kirk ||
|| State || Review ||
== Objectives ==
{{{
952: FR 1511, added localization for numbers in webpages. Almost sure Im done
}}}
'''General comments''':
|| '''Description''' || '''Classification''' || '''Status''' ||
|| I think the following places are missing:
BitpreserveFileState.printMissingFileState()
BitpreserveFileState.printChecksumErrorState()
Definitions-domain-statistics.jsp (the count local variable)
Translations.properties needs t obe checked for places where numbers are part of the formatted string, for instance "current.list.contains.0.urls" in the viewerproxy || Minor || OK ||


=== Comments on file 'trunk/webpages/HarvestDefinition/Definitions-edit-snapshot-harvest.jsp', revision 952 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 99-100, 150 || As above, consider utility method. || Cosmetic || OK ||
=== Comments on file 'trunk/webpages/HarvestDefinition/Definitions-snapshot-harvests.jsp', revision 952 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
=== Comments on file 'trunk/webpages/HarvestDefinition/Definitions-edit-domain-config.jsp', revision 952 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 69 || What happened here? It looks like to methods of listing imports are mixed up, and one breaks the "max 80 chars" rule || Cosmetic || OK ||
|| 112, 146 || The changes to allow formatting numbers seems a little scattered all over the place. One varaible is initialised here, one near the first use. If loc actually necessary, or could it be inlined into a single intialisation looking like NumberFormat nf = NumberFormat.getInstace(HTMLUtils.getLocaleObject(pageConext));? || Cosmetic || OK ||
|| 112, 146, 150-151 || Suggestion: Should formatting numbers with localization be made into a static  utility method in HTMLUtils? || Cosmetic || OK ||
|| 151 || Wait, this doesn't make sense at all, does it? This is part of building a URL, why would you want to localize that? || Minor || OK ||
=== Comments on file 'trunk/webpages/HarvestDefinition/Definitions-add-event-seeds.jsp', revision 952 ===
|| '''Lines''' || '''Description''' || '''Classification''' || '''Status''' ||
|| 130 || As in the last JSP file, should this be an utility method? || Cosmetic || OK ||