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

Added xml reader hyperlink support #223

Closed
wants to merge 9 commits into from
Closed

Added xml reader hyperlink support #223

wants to merge 9 commits into from

Conversation

yasar-luo
Copy link
Contributor

@yasar-luo yasar-luo commented Sep 18, 2017

This is:

  • a bugfix
  • a new feature

Checklist:

What does it change?
Add XML Reader hyperlink support so that we can use link for the cell.

@PowerKiKi
Copy link
Member

Tests and changelog are missing, so it can't be merge yet.

@PowerKiKi PowerKiKi added the reader/xml Reader for MS SpreadsheetML spreadsheet files label Sep 18, 2017
@yasar-luo
Copy link
Contributor Author

@PowerKiKi Hello, this branch seems out of time with GreatHumorist:develop918_1, but there is just one line conflict, the code I have tested in local and it works, please help me resolve the conflict, thx.

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.

Looks mostly good to me. There is one more thing I didn't mention yet, it's the feature cross reference table, that we should try to keep up to date when add new features. It should most likely be that line that need to be changed. You can compile the documentation with MkDocs to double check.

@@ -549,7 +549,9 @@
<Data ss:Type="String">AE</Data>
</Cell>
<Cell ss:StyleID="ce5"/>
<Cell ss:StyleID="ce5"/>
<Cell ss:StyleID="ce5" ss:HRef="http://www.phpexcel.net/">
<Data ss:Type="String">PHPExcel</Data>
Copy link
Member

Choose a reason for hiding this comment

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

Let's not use "PHPExel" anymore, but instead "http://www.example.com" and "My website" or something similar.

protected function loadXEETestFile()
{
if (!$this->spreadsheetXEETest) {
$filename = __DIR__ . '/../../../samples/templates/Excel2003XMLTest.xml';
Copy link
Member

Choose a reason for hiding this comment

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

because we chdir(__DIR__); in tests/bootstrap.php, you can simplify this line with:

$filename = '../samples/templates/Excel2003XMLTest.xml';


// Load into this instance
$reader = new Xml();
$this->spreadsheetXEETest = $reader->loadIntoExisting($filename, new Spreadsheet());
Copy link
Member

Choose a reason for hiding this comment

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

why not just $reader->load($filename) ?

@yasar-luo
Copy link
Contributor Author

@PowerKiKi updated

@PowerKiKi PowerKiKi closed this in 7aa6233 Sep 22, 2017
@yasar-luo yasar-luo deleted the develop918_2 branch September 22, 2017 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
missing test reader/xml Reader for MS SpreadsheetML spreadsheet files
Development

Successfully merging this pull request may close these issues.

2 participants