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

Taggle Fusion Canvas Texture Renderer #561

Open
wants to merge 19 commits into
base: taggle-fusion
Choose a base branch
from

Conversation

domdir
Copy link

@domdir domdir commented Sep 7, 2018

Related PR: Caleydo/lineup_app#19

Summary

Introduces a new overview renderer.
image

In order to display big datasets the columns are rendered onto a canvas. With this approach it is possible to give an overview over very large datasets.

The user is also able to define an expand area. This can be done bei holding down the ALT-key and draging a selection area:
image

The expand selection is displayed in a seperate column:
image

The user can expand the selected area to see the row details:
image

@domdir domdir requested a review from thinkh September 7, 2018 09:01
@thinkh thinkh changed the title Taggle fusion issue 3 4 Taggle Fusion Canvas Texture Renderer Sep 7, 2018
@thinkh
Copy link
Member

thinkh commented Oct 29, 2018

I get a runtime error when switching to the overview mode:

  1. npm start
  2. Open http://localhost:8080/builder4.html (or any other demo)
  3. Activate overview checkbox
Uncaught TypeError: Cannot set property 'innerHTML' of null
    at Taggle.setViolation (Taggle.ts:127)
    at Object.violationChanged (Taggle.ts:26)
    at TaggleRenderer.dynamicHeight (TaggleRenderer.ts:92)
    at Object.dynamicHeight (TaggleRenderer.ts:49)
    at EngineRenderer.heightsFor (EngineRenderer.ts:349)
    at EngineRenderer.render (EngineRenderer.ts:389)
    at rankings.forEach (EngineRenderer.ts:378)
    at Array.forEach (<anonymous>)
    at EngineRenderer.update (EngineRenderer.ts:377)
    at TaggleRenderer.update (TaggleRenderer.ts:163)

It seems that the <div class="${cssClass('rule-violation')}"></div> from line 92 is not set because of the surrounding if statement.

@@ -888,6 +996,14 @@ abstract class ADataProvider extends AEventDispatcher implements IDataProvider {
this.fire(ADataProvider.EVENT_SELECTION_CHANGED, [], false);
}

clearDetail() {
Copy link
Member

Choose a reason for hiding this comment

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

Please add function comment

@@ -877,6 +981,10 @@ abstract class ADataProvider extends AEventDispatcher implements IDataProvider {
return Array.from(this.selection);
}

getDetail() {
Copy link
Member

Choose a reason for hiding this comment

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

Please add function comment

@@ -869,6 +966,13 @@ abstract class ADataProvider extends AEventDispatcher implements IDataProvider {
return this.view(this.getSelection());
}

detailRows(): Promise<any[]> | any[] {
Copy link
Member

Choose a reason for hiding this comment

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

Please add function comment

@@ -858,6 +945,16 @@ abstract class ADataProvider extends AEventDispatcher implements IDataProvider {
this.fire(ADataProvider.EVENT_SELECTION_CHANGED, this.getSelection());
}

removeDetailAll(indices: number[]) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add function comment

@@ -832,6 +894,23 @@ abstract class ADataProvider extends AEventDispatcher implements IDataProvider {
return true;
}

toggleDetail(index: number, additional = false) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add function comment

@@ -844,6 +923,14 @@ abstract class ADataProvider extends AEventDispatcher implements IDataProvider {
this.fire(ADataProvider.EVENT_SELECTION_CHANGED, this.getSelection());
}

removeDetail(index: number) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add function comment

@@ -809,6 +860,17 @@ abstract class ADataProvider extends AEventDispatcher implements IDataProvider {
this.selectAll(indices);
}

setDetail(indices: number[]) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add function comment

@@ -790,10 +827,24 @@ abstract class ADataProvider extends AEventDispatcher implements IDataProvider {
this.fire(ADataProvider.EVENT_SELECTION_CHANGED, this.getSelection());
}

addDetailAll(indices: number[]) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add function comment

@@ -758,6 +787,14 @@ abstract class ADataProvider extends AEventDispatcher implements IDataProvider {
this.fire(ADataProvider.EVENT_SELECTION_CHANGED, this.getSelection());
}

addDetail(index: number) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add function comment

@@ -743,6 +768,10 @@ abstract class ADataProvider extends AEventDispatcher implements IDataProvider {
return this.selection.has(index);
}

isDetail(index: number) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add function comment

@@ -160,6 +172,8 @@ abstract class ADataProvider extends AEventDispatcher implements IDataProvider {
*/
private readonly selection = new OrderedSet<number>();

private readonly detail = new OrderedSet<number>();
Copy link
Member

Choose a reason for hiding this comment

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

Add property comment

@import './threshold';
@import './upset';
@import './verticalbar';
.#{$lu_css_prefix} {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this necessary?

@@ -163,7 +163,7 @@ export default class EngineRanking extends ACellTableSection<RenderColumn> imple
}
};

constructor(public readonly ranking: Ranking, header: HTMLElement, body: HTMLElement, tableId: string, style: GridStyleManager, private readonly ctx: IEngineRankingContext, roptions: Partial<IEngineRankingOptions> = {}) {
constructor(public readonly ranking: Ranking, header: HTMLElement, body: HTMLElement, tableId: string, style: GridStyleManager, private readonly ctx: IEngineRankingContext, roptions: Partial<IEngineRankingOptions> = {}, readonly noEvents: boolean = false) {
Copy link
Member

Choose a reason for hiding this comment

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

Invert semantic of noEvents = false for easier reading. Because in line 188 the counter part is used if(!noEvents). Rename to addEventListener = true.

if (newValue) {
// become visible
const index = ranking.children.indexOf(col);
if (!noEvents) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment why and when no event listner is added? Otherwise nobody understands why this flag exists.

d3.select(this.node).select('main').style('display', 'none');
}

s2d() {
Copy link
Member

Choose a reason for hiding this comment

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

Do not use abbreviations in this case.

this.textureRenderer.s2d();
}

d2s() {
Copy link
Member

Choose a reason for hiding this comment

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

Do not use abbreviations in this case.

this.node.style.display = 'none';
}

s2d() {
Copy link
Member

Choose a reason for hiding this comment

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

Do not use abbreviations in this case.

this.engineRenderer.ctx.provider.setDetail(this.engineRenderer.ctx.provider.getSelection());
}

d2s() {
Copy link
Member

Choose a reason for hiding this comment

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

Do not use abbreviations in this case.


for (let i = fromIndex; i <= toIndex; i++) {
if (this.engineRenderer.ctx.provider.isSelected(this.currentLocalData[0][i].i)) {
ctx.fillStyle = '#ffa809';
Copy link
Member

Choose a reason for hiding this comment

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

Use class color constant

if (this.engineRenderer.ctx.provider.isSelected(this.currentLocalData[0][i].i)) {
ctx.fillStyle = '#ffa809';
} else {
ctx.fillStyle = '#ffffff';
Copy link
Member

Choose a reason for hiding this comment

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

Use class color constant

@@ -179,4 +188,12 @@ export default class TaggleRenderer extends AEventDispatcher {
enableHighlightListening(enable: boolean) {
this.renderer.enableHighlightListening(enable);
}

d2s() {
Copy link
Member

Choose a reason for hiding this comment

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

Do not use abbreviations in this case.

this.renderer.d2s();
}

s2d() {
Copy link
Member

Choose a reason for hiding this comment

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

Do not use abbreviations in this case.

@@ -305,6 +305,16 @@ export const toolbarActions: { [key: string]: IToolbarAction | IToolbarDialogAdd
const ss = new Set(s);
const others = order.filter((d) => !ss.has(d));
ctx.provider.setSelection(others);
}),
selectionToOverviewDetail: ui('S2D', (_col, _evt, ctx, level) => {
Copy link
Member

Choose a reason for hiding this comment

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

Use more explainable label

const s = ctx.provider.getSelection();
ctx.provider.setDetail(s);
}),
overviewDetailToSelection: ui('D2S', (_col, _evt, ctx, level) => {
Copy link
Member

Choose a reason for hiding this comment

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

Use more explainable label


group(row: IDataRow) {
const isSelected = this.getValue(row);
return isSelected ? OverviewDetailColumn.DETAILED_GROUP : OverviewDetailColumn.NOT_DETAILED_GROUP; }
Copy link
Member

Choose a reason for hiding this comment

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

Check formatting

private alreadyExpanded: boolean = false;
private expandLaterRows: any[] = [];
private readonly options: Readonly<ILineUpOptions>;
private readonly idPrefix = 'testprefix';
Copy link
Member

Choose a reason for hiding this comment

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

Why testprefix?

totalWidth += rankingWidth;
});
if (totalWidth > this.node.clientWidth) {
this.currentNodeHeight -= 20;
Copy link
Member

Choose a reason for hiding this comment

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

Where does this 20 px are coming from? Is there an existing constant that you can use?

Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

I couldn't test this functionality with the lineupjs demos, as there is a bug (as described above). Afterwards I can test it again.
Please fix the mentioned comments, before I'll merge the PR.

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.

2 participants