Re: [seam-dev] Excel module added to SVN

Guys, This stuff is great, thank you. I've got a few comments: * ExcelExporter's namespace isn't the same as its package name (it should be) * Create an import for org.jboss.seam.excel in the META-INF/ components.xml of the module to allow reference as just excelExporter * Reuse the uiComponent Seam component to look up the provided datatable * All Seam components should be declared @BypassInterceptors * Why are the Workbook implementation components Seam components? And the factory design feels wrong to me. I would suggest a class based scheme, with the ability to register extra types via components.xml: @Name("org.jboss.seam.excel.excelFactory") @Scope(ScopeType.STATELESS) @AutoCreate @BypassInterceptors public class ExcelFactory { private static Map<String, Class<? extends ExcelWorkbook>> defaultImplementations; static { defaultImplementations = new HashMap<String, Class<? extends ExcelWorkbook>>(); defaultImplementations.put("csv", CsvExcelWorkbook.class); defaultImplementations.put("jxl", JXLExcelWorkbook.class); } private Map<String, Class<? extends ExcelWorkbook>> implementations; // getter and setter for implementations public static ExcelFactory instance() { return (ExcelFactory) Component.getInstance(ExcelFactory.class); } public ExcelWorkbook getExcelWorkbook(String type) { if (Strings.isEmpty(type)) { type = "jxl"; } Class<? extends ExcelWorkbook> clazz; if (implementations != null) { clazz = implementations.get(type); } else { clazz = defaultImplementations.get(type; } ExcelWorkbook excelWorkbook = clazz.newInstance(); if (excelWorkbook == null) { throw new IllegalArguementException("Unable to create workbook of type " + type_; } return excelWorkbook; } } * Avoid custom exception types - why do we need them? * We need to be careful in the documentation to always refer to "the Microsoft (R) Excel (R) spreadsheet application" (Microsoft (R) Excel(R) doesn't cut it) - so I added &excel; and &Excel; macros to ensure we do this. * Also, there should be an 80 col gutter with the docs, and inline XML (like <literal>) should be placed inline * I get these errors when running the example: 12:35:24,567 WARN [JXLTemplates] Could not find cell template data, try 12:35:24,568 WARN [JXLTemplates] Could not find cell template global, try * Why have <e:headerFooter type="header">? Why not <e:header />, <e:footer /> - this seems to make much more sense to me * What is the point of <e:headerFooterCommands>? To allow multiple children of a facet? Why not use <ui:fragment /> * Why this weird headerCommand stuff? Why not do this normally? <e:header> <f:facet name="left"> This worksheet was created at #{time} on #{date}. It has #{total_page} pages. </f:facet> </e:header> with the CSS for altering the styling * We should look at moving xthe worksheet "commands" to be inline (like html) * I remain totally unconvinced by the template stuff - what does this solve that facelets doesn't in a neater, consistent way? * The CSS stuff for the datatable exporter is excellent. This is the way formatting should be specified on the custom tags too, and probably remove all the formatting attributes from the e: tags * The custom CSS names should be in line with standard css e.g. xlsFontName -> xls-font-family xlsFontSize -> xls-font-size It would also be great if you were able to support compound values as CSS does (e.g. xls-font: 9pt sans-serif) and also use standard CSS attributes if they are present e.g. on the datatable exporter <h:dataTable style="font: sans-serif 9pt; xls-font-size: 12pt;" /> would produce a sans-serif 9pt font in HTML and a 12pt sans-serif font on the spreadsheet. We also need to support externally defined CSS (via <link />) Sorry for the length of this :-) Pete On 1 Aug 2008, at 07:56, Daniel Roth wrote: