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

Remove one (redundant) line from Chart/Axis.php setAxisOptionsProperties(); change default value of $crossBetween to 'midCat' #2932

Closed
2 of 11 tasks
bridgeplayr opened this issue Jul 11, 2022 · 6 comments

Comments

@bridgeplayr
Copy link

bridgeplayr commented Jul 11, 2022

This is:

What is the expected behavior?

What is the current behavior?

What are the steps to reproduce?

Please provide a Minimal, Complete, and Verifiable example of code that exhibits the issue without relying on an external Excel file or a web server:

Also - while you're in there making changes, change default value of $crossBetween because a non-zero value of minimum or maximum will not take effect unless $crossBetween is either midCat or 'between'

WAS: private $crossBetween = ''; // 'between' or 'midCat' might be better
IS: private $crossBetween = 'midCat'; //

`

    $this->setAxisOption('major_tick_mark', $majorTmt);
    $this->setAxisOption('minor_tick_mark', $minorTmt); //<---
    $this->setAxisOption('minor_tick_mark', $minorTmt);
    $this->setAxisOption('minimum', $minimum);`

If this is an issue with reading a specific spreadsheet file, then it may be appropriate to provide a sample file that demonstrates the problem; but please keep it as small as possible, and sanitize any confidential information before uploading.

What features do you think are causing the issue

  • Reader
  • Writer
  • Styles
  • Data Validations
  • Formula Calulations
  • Charts
  • AutoFilter
  • Form Elements

Does an issue affect all spreadsheet file formats? If not, which formats are affected?

Which versions of PhpSpreadsheet and PHP are affected?

1.24 7.3

@bridgeplayr bridgeplayr changed the title Remove one (redundant) line from Chart/Axis.php setAxisOptionsProperties() Remove one (redundant) line from Chart/Axis.php setAxisOptionsProperties(); change default value of $crossBetween to 'midCat' Jul 11, 2022
@oleibman
Copy link
Collaborator

Changing the default value for crossBetween to null string gave much better results with the sample files. It is likely that none of the improved files specified min or max; nevertheless, I doubt that I will want to change the default back. It could make sense for the writer to use a default of midCat if it is null and min or max is non-zero. Please make that case, preferably with test file(s) to demonstrate the difference; and also please make the case for midCat being better than between (or for either of them being better than just documenting this requirement and relying on the user to read the documentation and add the additional statement).

Did you mean to close #2929?

@bridgeplayr
Copy link
Author

I will examine sample files for impact of 'crossBetween'.

In my project, Excel refused to display my generated ss after I assigned 'min'/'max' axis values. When I allowed Excel to repair it, the key change made by Excel was the assignment of value 'midCat' to crossBetween. Therefore, I will add 'min'/'max' values to axes in selected samples to observe the impact.

I did not mean to close the issue. I meant only to close the Comment. I am largely baffled by Github terminology. Is it more appropriate to create a Pull Request? It would seem that a user submits an ISSUE when the user has no implementation/correction suggestions. Is it then more appropriate to create a PR when the user has suggested implementation code?

My style is to change my version of the library code to suit my needs, then submit an ISSUE rather than the seemingly scary PR. This scheme runs the risk of falling behind on released versions, which I understand. I considered creating a fork for my work, but file management made me hesitate to do that.

oleibman added a commit to oleibman/PhpSpreadsheet that referenced this issue Jul 12, 2022
Fix PHPOffice#2931. If surface charts are written with c:grouping tag, Excel will treat them as corrupt. Also, some 3D-ish Xml tags are required when 2D surface charts are written, else they will be treated as 3D. Also eliminate a duplicate line identified by PHPOffice#2932.
@oleibman oleibman mentioned this issue Jul 12, 2022
7 tasks
@oleibman
Copy link
Collaborator

Note that 2933 will eliminate the redundant line.

A Pull Request is code ready to be installed; you will, presumably, have forked the main project, created a new branch, made your fixes, run the unit test suite successfully, and committed and pushed the change. If you aren't doing all of this, then opening an issue to report the problem is the appropriate action, whether or not you have ideas on how it should be fixed. We welcome PRs, and try to work with you on them if we feel additional changes are required. As an example, if you had submitted eliminating the duplicate line as a PR, we would probably have merged that pretty quickly. On the other hand, if you had submitted a PR to change the default for crossBetween, we'd be having the same discussion in the PR as we are now having in the issue. Actually, our preferred practice if you want to submit a PR is to open an issue and a PR whose commit message mentions that it fixes the issue; that would probably be overkill for the duplicate line, but would be sensible for crossBetween.

@oleibman
Copy link
Collaborator

I took sample file 32readwriteScatterChart2.xlsx, set minimum and maximum, and saved to a new file. I then edited the xml, deleting the crossBetween statements, so that the xml now contains c:min and c:max without c:crossBetween. I had no trouble opening this file. So, whatever the problem you saw in your corrupt file, the absence of crossBetween was either not a problem at all or, at least, only part of the problem.

@bridgeplayr
Copy link
Author

I have been unable to reproduce the behavior by Excel that I observed in my project, nor in several sample chart creators. Please close this issue. Sorry for wasting your time.

@oleibman
Copy link
Collaborator

Closing as requested. I will be looking into your gradient request soon.

oleibman added a commit that referenced this issue Jul 17, 2022
Fix #2931. If surface charts are written with c:grouping tag, Excel will treat them as corrupt. Also, some 3D-ish Xml tags are required when 2D surface charts are written, else they will be treated as 3D. Also eliminate a duplicate line identified by #2932.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants