Refactor word wrapping and on-node properties #817
Conversation
Document graphfields param
Document graphfields param
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #817 +/- ##
============================================
- Coverage 40.12% 39.68% -0.44%
+ Complexity 2332 2329 -3
============================================
Files 78 78
Lines 8247 8322 +75
============================================
- Hits 3309 3303 -6
- Misses 4938 5019 +81 ☔ View full report in Codecov by Sentry. |
|
@YOUR1, please take part in the review. |
| @@ -118,7 +119,7 @@ class GraphFormatterTest extends \PHPUnit\Framework\TestCase { | |||
| * @return GraphFormatter | |||
| */ | |||
| private static function graph( array $case ): GraphFormatter { | |||
| $graph = new GraphFormatter( new GraphOptions( GraphFormatterTest::BASE_PARAMS + $case['params'] ) ); | |||
| $graph = new GraphFormatter( new GraphOptions( array_merge( GraphFormatterTest::BASE_PARAMS, $case['params'] ) ) ); | |||
There was a problem hiding this comment.
Is there any reason why you changed this?
There was a problem hiding this comment.
+ was an error. In this place $case['params'] should override GraphFormatterTest::BASE_PARAMS.
src/Graph/GraphFormatter.php
Outdated
| . '</td></tr>'; | ||
| }, $fields ) ) . "\n</table>\n>"; | ||
| $label = $nodeLabel ?: $node->getID(); | ||
| $nodeTooltip = htmlspecialchars( $label ); |
There was a problem hiding this comment.
The previous version replaced the <br /> with '' when the Diagrams extension was loaded if im correct. What happened to that?
There was a problem hiding this comment.
Please elaborate. Replaced where?
There was a problem hiding this comment.
The previous version replaced the '' with '' when the Diagrams extension was loaded if im correct. What happened to that?
I presume you mean the tooltip. OK, I have wrapped it with $this->getWordWrappedText().
There was a problem hiding this comment.
Sorry for the confusion; Github did a weird thing with my <br />.
You refactored this part;
if ( version_compare( $wgVersion, '1.33', '>=' ) &&
\ExtensionRegistry::getInstance()->isLoaded( 'Diagrams' ) ) {
$nodeTooltip = str_replace( '<br />', '', $nodeTooltip );
}But I can't find that anywhere now.
There was a problem hiding this comment.
So, node tooltip for table nodes (with fields) for Diagrams should have line breaks instead of <br />?
There was a problem hiding this comment.
Also, a question: could you please check how Diagrams will treat \n escape sequence in a quoted node label or identifier, like [label="First line\nsecond line"] or "First line\second line" -> 'Other node"?
$this->getWordWrappedText( )
|
Can we merge this, @YOUR1 ? |
No, we shouldn't. I am abandoning this request. I have fully rewritten the whole format, only unit tests are remaining. |
|
Abandoning in favour of a more significant change. |
Refactor word wrapping and on-node properties, to facilitate further changes.