Skip to content

Conversation

@lumos309
Copy link
Contributor

Issue #451: List viewport not resizing in y-direction.

Issue was caused by a previous bug fix that enabled overflow in the list visualiser panel, but limited the maximum height of the panel itself. Since overflow and scrolling now works properly even without the bug fix (tested on Chrome and Edge), it can be commented out to fix this current issue.

@coveralls
Copy link

coveralls commented Feb 28, 2019

Pull Request Test Coverage Report for Build 1239

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 7.851%

Totals Coverage Status
Change from base Build 1202: 0.0%
Covered Lines: 280
Relevant Lines: 2994

💛 - Coveralls

@lumos309 lumos309 changed the title Fix for issue #451 Fix for issues #451 & #454 Feb 28, 2019
@lumos309
Copy link
Contributor Author

Fix for Issue #454 also: visualizer.js script now calculates the height of the list before using it to create the Kinetic Stage.

@jiachen247 jiachen247 self-requested a review March 1, 2019 10:03
@jiachen247
Copy link
Contributor

Can you yarn format and push again please! @lumos309

@lumos309
Copy link
Contributor Author

lumos309 commented Mar 2, 2019

@jiachen247 done!

Copy link
Contributor

@jiachen247 jiachen247 left a comment

Choose a reason for hiding this comment

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

Generally looks okay but for some reasons the CI is still breaking please make sure you yarn format and ensure all test cases are passing before pushing. Are you pushing with the no-verify flag?

@lumos309
Copy link
Contributor Author

lumos309 commented Mar 3, 2019

@jiachen247 I made the changes you suggested and also edited the commenting style in the .scss file, and now it passes! yarn format didn't pick up on any problems, but I suspect it was an extra newline and/or using // in consecutive lines.

@martin-henz
Copy link
Member

Hmm, trying to build and run, but am getting this. Any idea what's happening?

TypeError: Cannot read property 'init' of undefined
ListVisualizer.componentDidMount
src/components/workspace/side-content/ListVisualizer.tsx:8
5 |
6 | public componentDidMount() {
7 | if (this.$parent) {

8 | (window as any).ListVisualizer.init(this.$parent);
9 | }
10 | }
11 |
View compiled
▶ 13 stack frames were collapsed.

jiachen247
jiachen247 previously approved these changes Mar 4, 2019
@jiachen247 jiachen247 self-requested a review March 4, 2019 08:19
@jiachen247 jiachen247 dismissed their stale review March 4, 2019 08:20

missed width.

*/
function draw(xs) {
stage = new Kinetic.Stage({
width: 1000,
Copy link
Contributor

@jiachen247 jiachen247 Mar 4, 2019

Choose a reason for hiding this comment

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

Can I ask why width defaults to 1000? With simple lists, this creates some padding on the right that shouldnt be there. It would be good if we could dynamically calculate the width at runtime as well!

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really sure how to go about finding the width. It seems like it would be tricky for lists that contains pointers to other lists...

Copy link
Member

Choose a reason for hiding this comment

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

I think you can find the width of the required window by visiting the lowest node on the right fringe of the tree. You start at the root and go right at each step until you reach a leaf. I think with the current tree drawing algorithm, you will find that that leaf is the node with the rightmost position.

Copy link
Contributor Author

@lumos309 lumos309 Mar 4, 2019

Choose a reason for hiding this comment

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

The problem is drawings like these, where there are both left and right nodes to account for, as well as multiple references to the same object. Nonetheless I think I may have a solution, which I'm working on now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've sorted it out! Both width and height are now calculated dynamically.

Copy link
Member

Choose a reason for hiding this comment

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

The problem is drawings like these, where there are both left and right nodes to account for, as well as multiple references to the same object. Nonetheless I think I may have a solution, which I'm working on now.

Wow, that's a weird example. Great that you worked it out.

@martin-henz
Copy link
Member

For this program

const a = list(list(1, 2, list(3), list(4, 5), pair(6, 7)), 2, 3);
const b = list(a, a, 10, 11, a);
draw_list(b);

I get:

Line 3: ReferenceError: is_empty_list is not defined

Note that we have replaced the function is_empty_list with is_null.

@lumos309
Copy link
Contributor Author

lumos309 commented Mar 4, 2019

Oh right, I'd forgotten about that. Updated.

Copy link
Member

@martin-henz martin-henz left a comment

Choose a reason for hiding this comment

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

Working fine.

@martin-henz martin-henz merged commit c306de6 into master Mar 5, 2019
@jiachen247
Copy link
Contributor

Good job @lumos309!!

@lumos309
Copy link
Contributor Author

lumos309 commented Mar 6, 2019

Thanks @jiachen247 ! I can delete the branch now right?

@martin-henz
Copy link
Member

Please don't delete the branch. I have merged it already with the master branch. So you can just checkout the master branch, and your changes should be in there. We keep merged branches in the repo, for future reference.

@geshuming geshuming deleted the list-visualizer branch June 8, 2019 02:42
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.

5 participants