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

Add id to table rows by using table id and row id #23

Closed
wants to merge 1 commit into from

Conversation

ThaiWood
Copy link

This allows linking directly to table rows, please let me know if further changes are wanted.

Copy link
Owner

@fabacab fabacab left a comment

Choose a reason for hiding this comment

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

Hey, this looks great, thank you for the patch. Please have a look at line 595 and let me know if you can adjust it as requested.

@@ -592,7 +592,8 @@ private function dataToHtml ($r, $options, $caption = '') {
$id = ( 0 === $this->invocations )
? 'igsv-' . $this->getDocId( $options['key'] )
: "igsv-{$this->invocations}-" . $this->getDocId( $options['key'] );
$html = '<table id="' . esc_attr( $id ) . '"';
$id = esc_attr( $id );
$html = '<table id="$id"';
Copy link
Owner

Choose a reason for hiding this comment

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

Unless I am misreading this, this hunk looks equivalent to the original. Why the change? If the functional execution of this hunk is the same as the original, please leave the original in place and remove this hunk from the commit. Thanks.

@ThaiWood
Copy link
Author

ThaiWood commented Feb 1, 2018

I've reverted that section, please take another look

@fabacab
Copy link
Owner

fabacab commented Feb 1, 2018

@ThaiWood Thanks for the update. Lastly, please clean this branch. Something like this should do it, I think:

git checkout -b pr-23 fabacab/master # Make a new branch based on fabacab's "master" branch
git merge --squash add-row-id       # Prepare a squashed commit of all the commits in the local `add-row-id` branch.
git commit -m "Add id to table rows by using table id and row id" # Commit that change set.
git push origin +pr-23:add-row-id   # Forcibly overwrite the remote's ("origin") "add-row-id" branch with the contents of the new "pr-23" branch.

@ThaiWood
Copy link
Author

ThaiWood commented Feb 5, 2018

squashed and overwritten as requested

@ThaiWood
Copy link
Author

ThaiWood commented Feb 6, 2018

Fixed a typo in 616 where the quotes didn't match, and resquashed

@fabacab
Copy link
Owner

fabacab commented Feb 6, 2018

This was merged but the git svn command rewrote the commit for compatibility with WordPress.org's Subversion server as 91f5192. Thanks and props to your @ThaiWood user account on WordPress.org were provided in the change log and the upgrade notice. :) Many thanks for your contribution, Thai.

@fabacab fabacab closed this Feb 6, 2018
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.

None yet

2 participants