Skip to content

Support pageOrder in page setup for Xlsx/Xls Readers/Writers, and implement basic page setup support for other Readers/Writers #1559

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

Merged
merged 31 commits into from
Jul 26, 2020

Conversation

MarkBaker
Copy link
Member

@MarkBaker MarkBaker commented Jun 29, 2020

This is:

- [ ] a bugfix
- [X] a new feature

Checklist:

Why this change is needed?

Simple resolution for Issue #1528

Page Order implemented for:

  • Xlsx Reader and Writer
  • Xls Reader and Writer

Basic page setup also implemented for:

  • Ods Reader
    • Orientation
    • Scale
    • Horizontal/Vertical Centering
    • Page Order
    • Page Margins
  • Gnumeric Reader
    • Orientation
    • Scale
    • Horizontal/Vertical Centering
    • Page Order
    • Page Margins
  • Xml Reader
    • Orientation
    • Scale
    • Horizontal/Vertical Centering
    • Page Order
    • Page Margins

@MarkBaker MarkBaker marked this pull request as draft June 30, 2020 15:48
@MarkBaker MarkBaker self-assigned this Jun 30, 2020
@MarkBaker MarkBaker changed the title Support pageOrder in page setup for Xlsx Reader and Writer Support pageOrder in page setup for Xlsx/Xls Readers/Writers, and implement basic page setup support for other Readers/Writers Jun 30, 2020
@PowerKiKi
Copy link
Member

@MarkBaker I see you regularly commit code style changes because of PHP-Cs-Fixer. Locally I use a git hook to automatically apply code style before committing. Avoiding the hassle of doing that manually. I suggest you set up something similar, so you can focus on more important things.

My setup is in .git/hooks/pre-commit:

#!/bin/bash

pass=true

files=$(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.(php|phtml)$')
if [ "$files" != "" ]; then

    # Run php syntax check before commit
    while read -r file; do
        php -l "$file"
        if [ $? -ne 0 ]; then
            pass=false
        fi
    done <<< "$files"

    # Run php-cs-fixer validation before commit
    echo "$files" | xargs ./vendor/bin/php-cs-fixer fix --diff --config .php_cs.dist
    if [ $? -ne 0 ]; then
        pass=false
    fi

    # Automatically add files that may have been fixed by php-cs-fixer
    echo "$files" | xargs git add
fi

if $pass; then
    exit 0
else
    echo ""
    echo "PRE-COMMIT HOOK FAILED:"
    echo "Code style validation failed. Please fix errors and try committing again."
    exit 1
fi

@MarkBaker MarkBaker added reader/gnumeric Reader for Gnumeric spreadsheet files reader/ods Reader for Open/LibreOffice spreadsheet files (OASIS) reader/xls Reader for MS BIFF-format (xls) spreadsheet files reader/xlsx Reader for MS OfficeOpenXML-format (xlsx) spreadsheet files reader/xml Reader for MS SpreadsheetML spreadsheet files labels Jul 4, 2020
@MarkBaker MarkBaker marked this pull request as ready for review July 5, 2020 19:15
@MarkBaker MarkBaker requested a review from PowerKiKi July 14, 2020 18:17
Copy link
Member

@PowerKiKi PowerKiKi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark as the other PR. Shall we try to use \PhpOffice\PhpSpreadsheetTests\Functional\AbstractFunctional to avoid depending on existing binary files ?

@MarkBaker
Copy link
Member Author

Again, I'm disinclined to use AbstractFunctional because it only really proves that we can read a file that we've generated ourselves, and not one generated by MS Excel or other external spreadsheet program. I think that as long as we keep the binaries small, and then they are justified for testing Readers

@MarkBaker MarkBaker requested a review from PowerKiKi July 19, 2020 10:50
@PowerKiKi
Copy link
Member

Fair enough, then it's good to merge for me.

@PowerKiKi PowerKiKi merged commit bd66a58 into master Jul 26, 2020
@PowerKiKi PowerKiKi deleted the Page-Setup-Page-Order branch July 26, 2020 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reader/gnumeric Reader for Gnumeric spreadsheet files reader/ods Reader for Open/LibreOffice spreadsheet files (OASIS) reader/xls Reader for MS BIFF-format (xls) spreadsheet files reader/xlsx Reader for MS OfficeOpenXML-format (xlsx) spreadsheet files reader/xml Reader for MS SpreadsheetML spreadsheet files
Development

Successfully merging this pull request may close these issues.

2 participants