Page MenuHomePhabricator
Differential D14354

Add scroll wrapper for big tables
ClosedPublic
Actions

Authored by chad on Oct 27 2015, 8:17 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 23, 5:03 AM
Unknown Object (File)
Fri, Dec 20, 5:05 PM
Unknown Object (File)
Fri, Dec 20, 11:11 AM
Unknown Object (File)
Mon, Dec 16, 2:34 PM
Unknown Object (File)
Sat, Dec 14, 3:16 PM
Unknown Object (File)
Sat, Dec 14, 11:25 AM
Unknown Object (File)
Sat, Dec 14, 7:24 AM
Unknown Object (File)
Wed, Dec 4, 6:07 PM
View All Files
Subscribers
Korvin

Details

Reviewers
epriestley
Maniphest Tasks
T9609: Really really wide tables in Remarkup should scroll
Commits
rPHUa1e8e2cd2042: (stable) Merge 2015 Week 44
rPHU59f5a8d2bb82: Add scroll wrapper for big tables
Summary

Adds a wrapping div to allow scrolling of overflowed tables

Test Plan

Really really really big table in Phriction, See scrollbar

Diff Detail

Repository
rPHU libphutil
Branch
table-wrap
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 8462
Build 9740: Run Core Tests
Build 9739: arc lint + arc unit

Event Timeline

chad updated this revision to Diff 34649.Oct 27 2015, 8:17 PM
chad retitled this revision from to Add scroll wrapper for big tables.
chad updated this object.
chad edited the test plan for this revision. (Show Details)
chad added a reviewer: epriestley.
Herald added a subscriber: Korvin. · View Herald TranscriptOct 27 2015, 8:17 PM
chad added a task: T9609: Really really wide tables in Remarkup should scroll.Oct 27 2015, 8:17 PM
epriestley requested changes to this revision.Oct 27 2015, 8:33 PM
epriestley edited edge metadata.

That test failure is probably really you? I'm guessing it's a test checking what the output of tables looks like? Or no?

This revision now requires changes to proceed.Oct 27 2015, 8:33 PM
chad added a comment.Oct 27 2015, 8:34 PM

Operator extension not available.

epriestley added a comment.Oct 27 2015, 8:35 PM

On Build 9730, it's got this:

   FAIL  PhutilRemarkupEngineTestCase::testEngine
182	Assertion failed, expected values to be equal (at PhutilRemarkupEngineTestCase.php:56): Failed to markup HTML in file 'newline-then-block.txt'.
183	Expected vs Actual Output Diff
184	--- Old Value
185	+++ New Value
186	@@ -1,8 +1,8 @@
187	 '<p>This is a paragraph.</p>
188	 
189	 <div class="remarkup-code-block" data-code-lang="txt" data-sigil="remarkup-code-block"><pre class="remarkup-code">First line of code block.
190	 Second line of code block.</pre></div>
191	 
192	-<table class="remarkup-table">
193	+<div class="remarkup-table-wrap"><table class="remarkup-table">
194	 <tr><td>Cell 1</td><td>Cell 2</td></tr>
195	-</table>'
196	+</table></div>'
197

...which is a legitimate failure (the markup emitted by tables has changed).

chad updated this revision to Diff 34651.Oct 27 2015, 8:36 PM
chad edited edge metadata.
  • add unit test
chad added a comment.Oct 27 2015, 8:37 PM

Updated the test, but unsure what's causing the test to not run.

epriestley added inline comments.Oct 27 2015, 8:41 PM
src/markup/engine/__tests__/remarkup/table.txt
1–7

This part shouldn't change -- it's the input (what you'd type in a comment area) and we don't expect users to type the <div /> part.

7–8

From the output, it looks like the actual markup that is produced is just like this:

<div ...><table ...>
...
</table></div>

...not on separate lines like it's written?

<div ...>
<table ...>
...
</table>
</div>
chad updated this revision to Diff 34652.Oct 27 2015, 8:41 PM
chad edited edge metadata.
  • add unit test
  • more tests
chad updated this revision to Diff 34653.Oct 27 2015, 8:45 PM
  • no idea
chad added a comment.Oct 27 2015, 8:45 PM
Running unit tests...
   FAIL  PhutilRemarkupEngineTestCase::testEngine
Assertion failed, expected values to be equal (at PhutilRemarkupEngineTestCase.php:56): Failed to markup HTML in file 'simple-table-with-link.txt'.
Expected vs Actual Output Diff
--- Old Value
+++ New Value
@@ -1,3 +1,3 @@
-'<table class="remarkup-table">
+'<div class="remarkup-table-wrap"><table class="remarkup-table">
 <tr><td><a href="http://example.com" class="remarkup-link" target="_blank" rel="noreferrer">name</a></td><td>[x]</td></tr>
-</table>'
+</table></div>'
chad updated this revision to Diff 34654.Oct 27 2015, 8:47 PM
  • all tests updated
chad added a comment.Oct 27 2015, 8:48 PM

ok, i'm bad at reading, didn't really it was a new test failing each fix.

epriestley accepted this revision.Oct 27 2015, 8:51 PM
epriestley edited edge metadata.
This revision is now accepted and ready to land.Oct 27 2015, 8:51 PM
Closed by commit rPHU59f5a8d2bb82: Add scroll wrapper for big tables (authored by chad, committed by chad). · Explain WhyOct 27 2015, 8:52 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
src/
markup/
engine/
__tests__/
remarkup/
4 lines
4 lines
4 lines
4 lines
remarkup/
blockrule/
3 lines

Diff 34654

View Options

src/markup/engine/__tests__/remarkup/newline-then-block.txt

Loading...
View Options

src/markup/engine/__tests__/remarkup/simple-table-with-link.txt

Loading...
View Options

src/markup/engine/__tests__/remarkup/simple-table.txt

Loading...
View Options

src/markup/engine/__tests__/remarkup/table.txt

Loading...
View Options

src/markup/engine/remarkup/blockrule/PhutilRemarkupBlockRule.php

Loading...
Phabricator · Built by Phacility