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

Prevent over-sizing of group heatmaps in nested columns #133

Merged
merged 7 commits into from
Feb 12, 2019

Conversation

thinkh
Copy link
Member

@thinkh thinkh commented Feb 4, 2019

closes Caleydo/lineup_app#46

prerequisites:

  • branch is up-to-date with the branch to be merged with, i.e. develop
  • build is successful
  • code is cleaned up and formatted
  • tested with Firefox 52, Firefox 57+, Chrome 64+, MS Edge 16+

Summary

The over-sizing is caused by the increased row height for aggregated groups.

Reducing the row height to the standard 18px will result in a normal heatmap width for nested columns.

image

As soon as I increase the height the width of the nested heatmaps increases exceeds the column.

image

By adding a width:100% the heatmaps stay within their column width.

image

However, now the gap between the nested columns disappeared. Hence, we must subtract the gap from the width to stay inline with the (nested column) grid.

image

With `width:100%` only the gap between the nested columns would disappear. Hence, we must subtract the gap from the width to stay inline with the (nested column) grid.

Caleydo/lineup_app#46
@thinkh thinkh requested a review from sgratzl February 4, 2019 17:04
@thinkh thinkh changed the title Set the width explicitly to prevent over-sizing Prevent over-sizing of group heatmaps in nested columns Feb 4, 2019
@sgratzl
Copy link
Member

sgratzl commented Feb 6, 2019

doesn't work for me, in Chrome I get:

image

i.e Sass is not translating this variable thus the whole rule is invalid and does nothing.

@sgratzl
Copy link
Member

sgratzl commented Feb 6, 2019

@thinkh
Copy link
Member Author

thinkh commented Feb 6, 2019

Thanks for pointing out. It's fixed now. I needed to escape the Sass variable.

@sgratzl
Copy link
Member

sgratzl commented Feb 7, 2019

one thing that I didn't get. when you tested this PR with the buggy CSS selector and still the behavior wasn't there anymore. How can this fix, fix the bug or is there a bug at all?

@thinkh
Copy link
Member Author

thinkh commented Feb 7, 2019

Well, I just didn't test it the way I should have tested it. 😄 But intesterstingly the sass-lint also didn't warn me... 🤔

@sgratzl
Copy link
Member

sgratzl commented Feb 7, 2019

can you please update the test file such that I can reproduce the initial bug

@thinkh
Copy link
Member Author

thinkh commented Feb 8, 2019

I added the groupBy to the HTML file. You can see the effect when disabling the newly added CSS width:

image

With the width it is correct:

image

@sgratzl sgratzl self-assigned this Feb 12, 2019
@sgratzl sgratzl merged commit edf026b into develop Feb 12, 2019
@sgratzl sgratzl deleted the thinkh/nested-column-stack-renderer branch February 12, 2019 08:43
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