Skip to content
This repository has been archived by the owner on Apr 23, 2024. It is now read-only.

feat: home button #120

Merged
merged 7 commits into from
Mar 12, 2020
Merged

feat: home button #120

merged 7 commits into from
Mar 12, 2020

Conversation

eliseeborn
Copy link
Collaborator

No description provided.

src/tree/render.js Outdated Show resolved Hide resolved
@@ -189,5 +199,5 @@ export function createContainer({
setExpandedState(newExpandedState);
}

return { svg, divBox, allNodes, positioning, width, height, element, zoomWrapper, tooltip };
}
return setContainerData({ svg, divBox, allNodes, positioning, width, height, element, zoomWrapper, tooltip, homeButton });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this just refactoring or do we need to call it here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So we need to set the container data again also when we click on the home button. We could set it there, but one place seems to be enough I would say

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was going off Niek's draft, haven't tested it with how it was originally. @niekvanstaveren do you know if it was necessary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the biggest issue here is the D3 zooming again. Because it stores the zoom state inside the element we need to clear the whole thing from the DOM. This is why we call createContainer from the onClick handler. You could set this up in the index file instead (which also places the button later on the DOM), but then we are putting more things in the index file again. Another option would be to use a fake state for this and change that state whenever you click on the button and add this state to the useEffect.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just moving the button creation to before the tooltip creation made it possible to remove z-index, at least

Copy link
Collaborator

Choose a reason for hiding this comment

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

But we were running setContainerData in index, it would not be adding anything?

Copy link
Collaborator

@niekvanstaveren niekvanstaveren Mar 10, 2020

Choose a reason for hiding this comment

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

Yes we were, and now we also need to setContainerData when you click on the button and run the function again. See row 116

@eliseeborn
Copy link
Collaborator Author

still have to style the svg icon so they match the home button in Sense

.html(`<span class="lui-fade-button__icon sn-org-home-icon">
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="20" height="20" viewBox="0 0 16 16">
<defs>
<path id="home-a" d="M2,7.9 L8,3.4 L14,7.9 L14,16 L12,16 L12,11.2 C12,11.0895431 11.9104569,11 11.8,11 L9.2,11 C9.08954305,11 9,11.0895431 9,11.2 L9,16 L2,16 L2,7.9 Z M7,13.8 L7,11.2 C7,11.0895431 6.91045695,11 6.8,11 L4.2,11 C4.08954305,11 4,11.0895431 4,11.2 L4,13.8 C4,13.9104569 4.08954305,14 4.2,14 L6.8,14 C6.91045695,14 7,13.9104569 7,13.8 Z M13,4.1 L16,6.5 L15,7.5 L8,2 L1,7.5 L0,6.5 L8,0 L11,2.4 L11,1 L13,1 L13,4.1 Z"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe store this and the chain selections button svg in a different file, wont clutter the codes as much?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's a good idea :)

Copy link
Collaborator

@haxxmaxx haxxmaxx left a comment

Choose a reason for hiding this comment

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

Actually kind of nice putting expandedState on the wrapper. fewer arguments :)

@eliseeborn eliseeborn merged commit 182894f into master Mar 12, 2020
@eliseeborn eliseeborn deleted the fdq/home-button branch March 12, 2020 09:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants