Skip to content
This repository was archived by the owner on Jul 1, 2025. It is now read-only.

adds classes to .d3plus-timeline based on presentation type#46

Open
cnavarreteliz wants to merge 2 commits intomainfrom
enhancement-45
Open

adds classes to .d3plus-timeline based on presentation type#46
cnavarreteliz wants to merge 2 commits intomainfrom
enhancement-45

Conversation

@cnavarreteliz
Copy link

closes d3plus/d3plus#747

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have documented all new methods.
  • I have written tests for all new methods/functionality.
  • I have written examples for all new methods/functionality.

src/Timeline.js Outdated

this._brushGroup = elem("g.brushGroup", {parent: this._group});
this._brushGroup.call(brush).transition(this._transition)
.attr("class", this._buttonBehaviorCurrent === "buttons" ? "d3plus-timeline-buttons" : "d3plus-timeline-ticks")
Copy link
Member

Choose a reason for hiding this comment

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

This logic does not work on re-renders, as you are changing the class from "brushGroup" to "d3plus-timeline-*", so that on re-render the selection for "g.brushGroup" will not work and it will create another group.

  1. Set the class to either "brushGroup buttons" or "brushGroup ticks".
  2. Move this attr setting before the previous transition (there can be funky behavior when transitioning a property that cannot be transitioned, like "class").

Copy link
Author

Choose a reason for hiding this comment

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

@davelandry I found a possible solution. The wrapper of the timeline by default is ticks. If we replace the class of ticks by buttons|ticks we'd solve the issue.
elem("g.ticks", {parent: this._group}).attr("class", this._buttonBehaviorCurrent);

I was testing and the shapes that we need to wrap don't related with brushGroup

Has it sense for you?

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.

conditionally add classes to .d3plus-viz-timeline based on presentation type

3 participants