search

misterspeedy / FsExcel

An F# Excel spreadsheet generator
MIT License
143 stars 18 forks source link

New Features Added: Individual Cell Sizing, Merged Cells & Vertical Alignment #38

Closed emigajda closed 1 year ago

emigajda commented 1 year ago

Hi Kit,

I have added the following features to your code:

One thing I did notice when creating the Vertical Alignment feature was that I could not use the following type names in the DU as follows:

type VerticalAlignment = | Bottom | Center | Top

due to the compiler confusing the names in the pattern matches on lines 457-472 in FsExcel.fs. For example, when wanting to use 'Center' in the DU for the VerticalAlignment, I kept getting the following error:

This expression was expected to have type 'VerticalAlignment'
but here has type 'HorizontalAlignment'

To resolve this, I would have needed to write in the code in lines 457-472:

match v with | VerticalAlignment.Bottom -> .... | VerticalAlignment.Center -> ... | VerticalAlignment.Top -> ...

(and the same for the HorizontalAlignment DU)

and when writing a program to use this feature I would need to write (for both Horizontal & Vertical Alignment):

HorizontalAlignment HorizontalAlignment.Center VerticalAlignment Center

HorizontalAlignment Right VerticalAlignment VerticalAlignment.Bottom

which is cumbersome and not consistent.

Hence, I chose the names:

type VerticalAlignment = | Base | Centre | TopMost

Please note the British English spelling of 'Centre'.

I completed your course "F Sharp from the Ground Up" in summer 2022 and hence I am still fairly new to F#, so I am not sure if this is an error on my part or an error with the F# compiler. I am happy to discuss this issue with you further should you wish.

Kind regards, Emilia

misterspeedy commented 1 year ago

@emigajda This is amazing work. I cannot thank you enough. I'll take me a few days to go through it but at first glance it looks perfect. I can't wait get this merged and try it out.

To find that you came did this after taking the course - well, wow.

Thanks again, I'll be back in touch soon.

emigajda commented 1 year ago

Hi Kit,

Thank you for your warm words and the commit. What have been your thoughts on the naming issue at a quick glance?

I have realised that my Merge feature is quite limited as stands and so I will be extending its functionality to allow for merging between named cells and merging cells given a depth/span from a particular starting cell (and vice versa). Hopefully I should have this ready within the next few days!

Kind regards, Emilia

misterspeedy commented 1 year ago

Great! I have a few things to tidy up which I can do separately over a similar timescale.

Once we have your stuff fully in - and given that (to be confirmed) ClosedXml no longer depends on System.Drawing.Common - I am thinking of calling this Release 1.0.0.

What do you think? No pressure on you re timescale.

Thanks!

On Fri, 24 Feb 2023 at 08:11, emigajda @.***> wrote:

Hi Kit,

I have realised that my Merge feature is quite limited as stands and so I will be extending its functionality to allow for merging between named cells and merging cells given a depth/span from a particular starting cell (and vice versa). Hopefully I should have this ready within the next few days!

Kind regards, Emilia

— Reply to this email directly, view it on GitHub https://github.com/misterspeedy/FsExcel/pull/38#issuecomment-1443066203, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGDPLWKVQGKFHFMKOC5G2DWZBULFANCNFSM6AAAAAAVDKW6CA . You are receiving this because you modified the open/close state.Message ID: @.***>

misterspeedy commented 1 year ago

Oh yes, and I'll have a think about the naming issue too.

On Fri, 24 Feb 2023 at 08:42, Kit Eason @.***> wrote:

Great! I have a few things to tidy up which I can do separately over a similar timescale.

Once we have your stuff fully in - and given that (to be confirmed) ClosedXml no longer depends on System.Drawing.Common - I am thinking of calling this Release 1.0.0.

What do you think? No pressure on you re timescale.

Thanks!

On Fri, 24 Feb 2023 at 08:11, emigajda @.***> wrote:

Hi Kit,

I have realised that my Merge feature is quite limited as stands and so I will be extending its functionality to allow for merging between named cells and merging cells given a depth/span from a particular starting cell (and vice versa). Hopefully I should have this ready within the next few days!

Kind regards, Emilia

— Reply to this email directly, view it on GitHub https://github.com/misterspeedy/FsExcel/pull/38#issuecomment-1443066203, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGDPLWKVQGKFHFMKOC5G2DWZBULFANCNFSM6AAAAAAVDKW6CA . You are receiving this because you modified the open/close state.Message ID: @.***>

misterspeedy commented 1 year ago

@emigajda I've fixed some failing tests, in case that is helpful.