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

Dynamically add tip labels #728

Merged
merged 3 commits into from
Jun 4, 2019
Merged

Conversation

kairstenfay
Copy link
Contributor

This PR addresses some of the issues stated in Issue #237, namely around tip labels appearing/disappearing dynamically.

The remaining work from that issue is on tip label transitioning and editing the hardcoded # of branches selected.

Because this issue was marked as high priority, I thought I'd open this PR in case the team wanted it merged before everything in the issue was resolved.

`useModifySVGInStages` is false unless `newLayout` is set to true. Thus,
set `useModifySVGInStages = newLayout`.
Previously, `updateTipLabels()` was not being called when modifying the
SVG after a layout change.
Previously, tip labels would not update in visibility unless the SVG or layout was modified, or the page was reloaded. Now tip labels reflect date range changes.
Copy link
Member

@tsibley tsibley left a comment

Choose a reason for hiding this comment

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

The new code and behaviour looks good to me! I was able to reproduce the tip-visibility issues on master and then see them fixed by this branch.

@@ -306,9 +306,6 @@ export const change = function change({
elemsToUpdate.add(".grid").add(".regression");
svgPropsToUpdate.add("cx").add("cy").add("d").add("opacity").add("visibility");
}
if (newLayout) {
useModifySVGInStages = true;
}

Copy link
Member

Choose a reason for hiding this comment

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

💯 This type of refactor is small but I think really helpful to iteratively making complex logic more understandable/approachable.

@trvrb
Copy link
Member

trvrb commented Jun 4, 2019

Thanks for cleaning this up. Looks good to me.

@kairstenfay kairstenfay merged commit c9ed89c into master Jun 4, 2019
@kairstenfay kairstenfay deleted the dynamically_add_tip_labels branch June 4, 2019 18:33
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.

3 participants