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

Feat/icicles d3 hover #1277

Open
wants to merge 1 commit into
base: feat/refactor-icicles-d3
Choose a base branch
from

Conversation

mehdilouraoui
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #1277 (0e9e9e8) into feat/refactor-icicles-d3 (f0040d1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                    Coverage Diff                    @@
##           feat/refactor-icicles-d3    #1277   +/-   ##
=========================================================
  Coverage                     80.64%   80.64%           
=========================================================
  Files                           121      121           
  Lines                          3069     3069           
  Branches                        410      409    -1     
=========================================================
  Hits                           2475     2475           
  Misses                          590      590           
  Partials                          4        4           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0040d1...0e9e9e8. Read the comment docs.

@mehdilouraoui mehdilouraoui changed the base branch from master to feat/refactor-icicles-d3 June 11, 2021 09:29
@@ -14,3 +14,8 @@ body {
h4 {
font-family: "Quicksand", sans-serif;
}

#icicles {
Copy link
Contributor

Choose a reason for hiding this comment

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

On n'utilise pas trop de scss d'habitude, on préfère le CSS in JS. Regarde si tu ne peux pas plutôt utiliser styled-components par exemple

@@ -0,0 +1,89 @@
import { ROOT_FF_ID } from "reducers/files-and-folders/files-and-folders-selectors";
import { FilesAndFolders } from "reducers/files-and-folders/files-and-folders-types";
import { fromFileName } from "./../../../util/color/color-util";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { fromFileName } from "./../../../util/color/color-util";
import { fromFileName } from "util/color/color-util";

(element) =>
element?.children.map((childId) => filesAndFolders[childId]) ?? []
)
.sum((d) => d?.file_size ?? 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remplacer d par un nom de variable plus explicite

.attr("width", ({ y0, y1 }) => y1 - y0 - 1)
.attr("height", (d) => getRectangleHeight(d))
.attr("fill-opacity", 0.5)
.attr("fill", (d: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Renommer d ici aussi, et le typer si possible

return fromFileName(d.data.name);
})
.style("cursor", "pointer")
.on("dblclick", (_, currentRect) => handleZoom(_, currentRect, elements));
Copy link
Contributor

Choose a reason for hiding this comment

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

Si handleZoom a besoin de _, ne pas nommer cette variable ainsi car elle est utilisée

.text((d: any) => d.data.name);

cell.append("title").text(
(d: any) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Idem, essayer de trouver un meilleur nom et pourquoi pas un type pour d

};

export const createSubtitle = (title) => {
return title
Copy link
Contributor

Choose a reason for hiding this comment

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

Peut se simplifier en retirant l'accolade et le return

dimensionsTarget[dimensions.data.id];

export const handleZoom = (_, currentRect, elements) => {
let { root, cell, rect, title, subtitle } = elements;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let { root, cell, rect, title, subtitle } = elements;
const { root, cell, rect, title, subtitle } = elements;

@@ -0,0 +1,79 @@
import * as d3 from "d3";
Copy link
Contributor

Choose a reason for hiding this comment

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

Déplacer ce fichier dans le dossier utils, et écrire des tests pour les fonctions exportées

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