Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement basic cell styling #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gafrom
Copy link

@gafrom gafrom commented Apr 17, 2021

Hi!

I built cell styling to be used in one of my projects. Then I thought maybe others will benefit from it too. So I added a few tests, and here it is, probably ready to be reviewed.

Please have a look at the implementation. It is not increasing latency/memory/disk.
If no styled cells are used, then the final XML is roughly same as by the original implementation.

@sandstrom
Copy link
Contributor

I understand if people disagree, but I don't think xlsxtream should add any visual styling capabilities. Setting cell type, e.g. number, string, percentage, etc. is fine, since that's more of a data concern.

But for visual styling, I think it's better to let other projects (or a fork) work on that. The primary reason being to keep the complexity down, which makes the project easier to maintain over time.

@gafrom
Copy link
Author

gafrom commented Jun 2, 2021

@sandstrom , my use case was to be able to use another "dimension" to represent data - I used background color of the cell to indicate the extent of an error for the data in the table. If I used another, say adjacent one or on another tab, then it would be a poor user experience for users of the data. So, the main purpose of this feature is not a beautification of the data, but rather a data concern.

That being said, I am not married to this pull request and totally understand the reasoning you gave. Please feel free to cancel the request, no problem.

@gafrom
Copy link
Author

gafrom commented Jun 2, 2021

Re: keeping the complexity down - to be honest, I found it simpler to have a class for styling to separate the concerns - rendering of basic styles that currently live in the row class.

@sandstrom
Copy link
Contributor

@gafrom I just wanted to provide another perspective. I'm not the repo owner, so it's not up to me to decide the PR's faith 😄

@pond
Copy link
Contributor

pond commented Jun 28, 2021

Yeah I mean I understand where @sandstrom is coming from but I'd find this quite useful myself. It's quite a lot of new code, but the majority of it only gets invoked if actually used and it seems to follow the naming, namespaces and general ethos of the original code base quite nicely.

This PR would allow me to achieve what I wanted to do in #44, but in a more flexible visual styling way rather than a semantic way. If the maintainer decides that this PR is useful, I think we could either close #44 entirely, or if we still liked the idea of a semantically defined "hear row" rather than having to style individual cells "manually", I could rework the concept of 'header row' into an automatically internally styled set of rows using your new classes rather than the simple, "MVP" extensions I put into #44.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants