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 |