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.

Sign up for GitHub

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

Jump to bottom

Sankey 3.2.2.0v #161

Merged
merged 27 commits into from
Sep 13, 2023
Merged

Sankey 3.2.2.0v #161

merged 27 commits into from
Sep 13, 2023

Conversation

kullJul
Copy link
Contributor

@kullJul kullJul commented Aug 30, 2023

No description provided.

@kullJul kullJul marked this pull request as ready for review August 30, 2023 18:15
snapp1
snapp1 approved these changes Aug 31, 2023
View reviewed changes
Copy link

@snapp1 snapp1 left a comment

Choose a reason for hiding this comment

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

LGTM, a lot of calc changed, test pass?

@kullJul
Copy link
Contributor Author

kullJul commented Aug 31, 2023

LGTM, a lot of calc changed, test pass?
yes, adding a link to the build job and a screenshot from running tests locally
https://github.com/microsoft/powerbi-visuals-sankey/actions/runs/6034458822/job/16372866878?pr=161
image

v-ruboranov
v-ruboranov reviewed Aug 31, 2023
View reviewed changes
src/sankeyDiagram.ts Outdated
@@ -214,6 +216,9 @@ export class SankeyDiagram implements IVisual {
values: "Weight"
};

public static InputTooltipValue: string = "Input ";
public static OutputTooltipValue: string = "Output ";
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need space in the end of the string?

Copy link
Contributor

Choose a reason for hiding this comment

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

these should be localized

src/sankeyDiagram.ts Outdated Show resolved Hide resolved
src/sankeyDiagram.ts Outdated

if (valueDisplayName) {
tooltips.push({
displayName: SankeyDiagram.InputTooltipValue + valueDisplayName || SankeyDiagram.RoleNames.values,
Copy link
Contributor

Choose a reason for hiding this comment

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

is '+' a good idea of string concatenation?

Copy link
Contributor

Choose a reason for hiding this comment

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

use backticks(`) like '${SankeyDiagram.InputTooltipValue} ${valueDisplayName}'
do not store whitespace inside of strings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed and localized 09b4985

src/sankeyDiagram.ts Outdated Show resolved Hide resolved
src/sankeyDiagram.ts Show resolved Hide resolved
MulyukovAidar
MulyukovAidar suggested changes Sep 4, 2023
View reviewed changes
src/behavior.ts Show resolved Hide resolved
src/behavior.ts Show resolved Hide resolved
src/sankeyDiagram.ts Outdated
@@ -214,6 +216,9 @@ export class SankeyDiagram implements IVisual {
values: "Weight"
};

public static InputTooltipValue: string = "Input ";
public static OutputTooltipValue: string = "Output ";
Copy link
Contributor

Choose a reason for hiding this comment

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

these should be localized

src/sankeyDiagram.ts Outdated

if (valueDisplayName) {
tooltips.push({
displayName: SankeyDiagram.InputTooltipValue + valueDisplayName || SankeyDiagram.RoleNames.values,
Copy link
Contributor

Choose a reason for hiding this comment

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

use backticks(`) like '${SankeyDiagram.InputTooltipValue} ${valueDisplayName}'
do not store whitespace inside of strings

src/sankeyDiagram.ts Outdated
link.dySource = shiftByAxisY;
else
link.dyDestination = shiftByAxisY;
link.dySource = shiftByAxisY;
Copy link
Contributor

Choose a reason for hiding this comment

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

what is dySource and dyDestination?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed 508dec9

src/sankeyDiagram.ts Outdated
@@ -1523,6 +1517,10 @@ export class SankeyDiagram implements IVisual {
.darker(SankeyDiagram.StrokeColorFactor)
.toString()
)
.attr("tabindex", 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

is tabindex = 0 applied to each node?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed b723383

src/sankeyDiagram.ts
x1 = link.destination.x;
}
y1: number,
curveRadius: number,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to set high value of curveRadius to selfLinks (so that there is no overlap between self link and backward link)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 9641777

style/visual.less Outdated
outline: none;
}

.nodeRect:focus-visible {
Copy link
Contributor

Choose a reason for hiding this comment

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

works on Edge, Chrome, Firefox?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

works as expected on Edge, Chrome and Firefox

style/visual.less Outdated
&:focus-visible {
outline: auto 1px;
outline-color: -webkit-focus-ring-color;
.link:focus {
Copy link
Contributor

Choose a reason for hiding this comment

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

try using less hierarchy like here
https://riptutorial.com/less/example/29941/pseudo-elements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed a68bc7a

MulyukovAidar
MulyukovAidar reviewed Sep 12, 2023
View reviewed changes
test/visualTest.ts
const links: NodeListOf<HTMLElement> = visualBuilder.linkElements;

links[0].dispatchEvent(enterEvent);

Copy link
Contributor

@MulyukovAidar MulyukovAidar Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

expect(links[0]...).toBeTrue();
const otherLinks = links.slice(1);
otherLinks.forEach((link)=>expect(link...).toBeFalse())

looks kinda more clear to me

@MulyukovAidar
Copy link
Contributor

LGTM

@MulyukovAidar MulyukovAidar merged commit 680a34f into microsoft:dev Sep 13, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MulyukovAidar MulyukovAidar MulyukovAidar requested changes

@v-ruboranov v-ruboranov v-ruboranov left review comments

@snapp1 snapp1 snapp1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kullJul @MulyukovAidar @snapp1 @v-ruboranov