Add support for series stroke factory into the base renderer class so subclasses don't need to recreate it #338
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… subclasses don't need to recreate it.
|
Updated to current master (as far as I can see) and fixed an IE incompatibility issue. Do you require any additional changes to merge this? Something to talk about: I was not particularly happy with the way the graph as a whole and the individual paths of each graph are represented on the data series, but didn't want to change that to not break backwards compatibility. What do you think about that? |
|
I like the spirit of this change but I'm reluctant to change the DOM structure across the board (moving from a single path by default to always having a group). It looks like this breaks toggling lines on and off (in the |
|
Well, I'm open to suggestions :-) What I'd like to achieve each of the created paths should be easy to access from inside the renderer without it having to be passed around. Additionally each series as a whole and each path in a series should be accessible via css to allow externalizing layout information. So maybe it would be best to have a css class on the series that is applied to both paths and then just add .stroke and .path to the two paths? That way saving them to the series isn't required (which is clumsy and could overwrite stuff already in there) and the extra group is also not required. I'm not sure if the renderer or the series should still get access to a direct reference to the rendered path elements to ease their handling? What do you think? Regards, |
|
I'd like to invest more time into this now because we need more access by css to the created dom elements. Are you ok with my proposal to just add the extra path and give them css classes to distinguish them? i.e. path.series-class.path and path.series-class.stroke as per the two path factories? |
|
That would mean no more extra groups and hopefully no changed dom. |
|
I'm starting to work on this now, if you would like to give input, please do so as soon as possible (as I'd like to spare duplicate work). |
|
How about this? Going without the dom modification but adding some classes to make it possible to target the individual paths from css so integrating applications can easily style them. |
|
At a glance I like this for now. I'll do some more testing and code review before merging in, but seems like this is going to work. Thanks! |
bohan
added a commit
to bohan/rickshaw
that referenced
this pull request
Aug 2, 2022
* Makes hover detail arrow rendering more reliable Uses the 'CSS triangle' trick to render the left and right arrows on the hover detail item, rather than using a glyph (whose rendering might change depending on the font used). The technique is described here: http://appendto.com/blog/2013/03/pure-css-triangles-explained/ * Improves hover detail placement when space is tight The layout error for hover detail elements is now checked for both left and right alignment. Then the alignment with the least error is used. * Add test case for Legend * Make Rickshaw.Graph.Legend a real class This makes it possible to extend the legend to add custom styling or interactivity. This commit should not change behavior or options. * Allow Legend subclasses to override the className * Extract legend rendering into render function that can be called multiple times * Return rendered line from addLine * tweak area renderer to play nicely with multi renderer; fixes shutterstock#335; closes shutterstock#336 * update built libs * Update README.md * excise Rickshaw.Graph.Unstacker and allow for consumers to pre-stack if they like; fixes shutterstock#333 * Add series configured class name to all scatterplot points * throw an error if provided element isn't an element * Added Rickshaw.Graph.Minimap class. The minimap will render exactly what the main graph renders, but will not pay attention to its "window" extents and thus will always show the whole data set. * refactor Minimap and rename to Rickshaw.Graph.RangeSlider.Preview; closes shutterstock#273 * add data attributes on axis ticks; closes shutterstock#347 * Update README.md * tell jshint to warn about leaking globals * be smarter about rounding months and years; closes shutterstock#344 * Update README.md * allow specifying custom x and y scales; closes shutterstock#346 * Respect series classes in legend renderer (to allow the color of the swatch to be controlled by css * invoke stroke factory in renderer parent class; closes shutterstock#337; closes shutterstock#338 * update built libs * support sending slide callbacks for range sliders; fixes shutterstock#363 * render annotations after adding each one * make sure to initialize the preview graphs' renderers to match parents for range slider previews; closes shutterstock#364 * fire range slider events with extents in tact * Rickshaw.Graph.RangeSlider.Preview supports millisecond zooming Changed the domainScale interpolate function to d3.interpolateNumber (vs. d3.interpolateRound) so that the RangeSlider Preview Window could zoom into millisecond levels of precision (before it would only allow a window of 1 second) * Fix shutterstock#377: Ensure multi renderer consults it's sub renderers about how they perceive their domain * Configure range slider from graph, so that width changes affect the slider width too. * Fix typo * Ensure Graph creates a copy of the x/yScale before modifying it x/yScale is coming from the configuration dictionary which may be referenced by the Graph creator, or shared with other Graphs. We need to ensure we copy the scale so that our mutations do not change the object given to us. * Prevent slider from cutting of series. This fixes the following bug: I have 4 series, [ [{0, 1}, {1, 1}], [{1, 1}, {2, 1}], [{2, 1}, {3, 1}], [{3, 1}, {4, 1}] ] I use a slider to select the x-range (2.5, 4) This means stackedData ends up being [ [], [], [], [[3,4], [1, 1]] ] At this point, reading stackedData[0][0] produces undefined, even though there is data that belongs to the range. * Recentered detail dot. explicitly set box-sizing. * refined margin-top * make jQuery dependent bits compatible with jQuery.noConflict(); fixes shutterstock#396 * Rescope stroke style attribute on y-axis In mbostock/d3 issue #1016, the scope of the `tick` class on a svg y-axis element changed to include the entire g object (line and text), not just the line. The Rickshaw CSS for `.y_ticks .tick` was created in the early commits, and sets the style to be a 2px width semitransparent stroke. Due to the change in scoping of the style class, the stroke element applies to both the line and the text element, creating a shadow on the y-axis text labels. By changing the scope of the stroke stanza from `.y_ticks .tick` to `.y_ticks .tick line`, the stroke now applies only to the line, and not to the text element, this removing the stroke shadow on the text label. * Extend Range Slider to accept multiple graphs Extend to allow array of graphs, and handle logic based on if the graph element is singular or plural. Also add examples/shared_slider.html as a demo of new functionality Similar to pullr equest shutterstock#169 (bozdoz) * Make jQuery known as the global symbol instead of $ To make it actually compile again. * Define $ variable in correct scope * Allow for developers to customise hook into jQUI slider events * Switch back to tabs * Add start and end scripts to allow use of UMD by Rickshaw * Allow empty data arrays in series. * Find correct domain even if the first series in stack is empty. This can easily happen when series of different length are drawn and the RangeSlider is used to restrict the graph to a range where one of the series is empty. * Removed trailing comma in example * provide a supported interface for toggling stacking * support resizing preview range sliders; closes shutterstock#375; closes shutterstock#417; closes shutterstock#382 * add x magnitude scale and improve barWidth calculation for bars renderer * use preview range slider in multi renderer example * refactor lineplot to work with multi renderer; closes shutterstock#413; closes shutterstock#414 * update built libs * Update README.md * Add README.md note about $super and minification; closes shutterstock#52 * Only update graph once when toggling a series, not once per series. * Remove duplicate graph update in smoother. * Update copyright year Fix outdated copyright year (update to 2014) The copyright year was out of date. Copyright notices must reflect the current year. This commit updates the listed year to 2014. see: http://www.copyright.gov/circs/circ01.pdf for more info * Better documentation around stacked and unstacked rendering. * Update example_04.html * Added method to remove HoverDetail's mouse listeners. * Show units when displaying minutes. When time is being displayed in minutes there is no indication of units. * Update README.md Per the actual source, one can pass an int to color() and select the nth color from a palette. I simply added this fact to the documentation. * transform either stroke or color properties on highlight * override which key to fetch legend swatch color from * Remove error if no jQuery. Uncaught ReferenceError: jQuery is not defined at line rickshaw.js:1985(anonymous function) * Add default value for `renderer` As much as I understand, `renderer` defaults to `line`. Adding that in the README * Add homepage, repository and issues links in package.json. * update built libs * fixing local time ticks * Fix typo * Make calculating interval in bar chart consistent. Fixes shutterstock#461 Sorting object's keys returned to guarantee consistency when iterating over. Keys order in `for .. in` loop is not specified and browsers behave differently here. This results with different invterval value being calculated for different browsers. See last but one section here: http://www.ecma-international.org/ecma-262/5.1/#sec-12.6.4. * HoverDetail should default to being inactive. * Add Rickshaw.Graph.Dragzoom * center x_axis on bar chart * Rename with .append at the end * Rename with .prepend at the end * use prepend/append files for UMD support prevents downstream issues from cropping up due to invalid JavaScript files. * RangeSlider: Resize when original graph changes width * bower ready, with d3 dependency * fix for 29 Feb * fixing license warning * added travis * upgrade dependancies * requiring minimally node4 to run tests jsdom works either with node <0.12 or with node >4 so this means you can install the package at node <4 but you wont be able to run the tests * add coverage * Multi-chart slider and colors for individual bars RangeSlider now works with multiple charts and a single slider, by using an array with the graph variables: var slider_three = new Rickshaw.Graph.RangeSlider({ element: document.querySelector('#slider-range'), graph: [graph_one, graph_two, graph_three] }); The bar chart renderer now looks for a proposed color for each individual bar, it will default to the chart-specific color if no color is declared: var data = [{ data: [{ x: -1893456000, y: 29389330}, { x: -1577923200, y: 33125803}, { x: -1262304000, y: 37857633, color:'#06f'}, { x: -946771200, y: 41665901, color:'#0cf'} ], color: 'shutterstock#222' }] * created an example with multiple graphs * multiple graphs with RangeSlider works * revert RangeSlider now works with multiple charts and a single slider, by using an array with the graph variables: 632d1fb * formatting and adding back $ to be able to build * building css * Version 1.6.0-rc.0 * Correct check for undefined variable The code was comparing the result of `typeof` with a variable named `undefined`. As typeof returns a string it should compare to `'undefined'` * added example bar chart * keeping the shared_slider example * Running `make watchjs` watches src folder * using npm run watch instead of makefile * added lint to package.json * consolodating code to avoid duplication * Version 1.6.0 * upgraded dependancies * support for node 4 * support for npm run watch while developing * RangeSlider can accept multiple graphs * fixed stacked area chart hover detail bug in firefox * Fixes issue with negative values between 0 and -1 Probably due to a typo, numbers between 0 and -1 were not formatted as expected. * Create a locally scoped copy of the loop-index var to prevent accessing out-of-bound index in the onConfigure callback * added tests for hover detail * added test for HoverDetail _removeListeners for shutterstock#477 * added tests for palette * added test for number formatting * added tests for annotate * updated coveralls badge link * Since JS doesn't have block scopes, only function scopes. So, in order to capture a loop-index variable for the later `onConfigure` function callback, we create an IIFE * added test for onConfigure for shutterstock#568 shutterstock#579 shutterstock#306 shutterstock#324 shutterstock#320 added resize in the series example to manually demo onConfigure * travis relies on built files * added test for DragZoom initialize error for shutterstock#512 and updated test to use jsdom and lint * including DragZoom in build * building if needed before running tests the tests run on the built code, rather than source code. building if needed helps ensure the test is running against changes and so that travis can run on prs * added tests for mouse events on dragzoom * added example for dragzoom * added Rickshaw.version fixes shutterstock#538 fixes shutterstock#224 * running make after version to have updated version in dist * 1.6.1 * based on series graph * angular example fixes shutterstock#374 and for shutterstock#477 * example of updating data via angular controller * instead of deap watch overwriting series data and adding method setSeries to update data fixes shutterstock#407 * linking to angular-rickshaw directive * Add ToC, Install section to README This pull request adds a Table of Contents using `doctoc`, adds an npm-script to easily run it, and adds an install section to the README that more accurately reflects how to use the package. I also moved dependencies up to the Install section, as this is probably where it belongs. * Edit CONTRIBUTING Right now, the Contributing file has nothing about READMEs. While shutterstock#588 has been opened to talk about labels, I thought it would be good to have something in the CONTRIBUTING file about labels and issues, in general. This adds that, and should not be too controversial. In particular, I want to field possible labels from those submitting issues. This should cut down on internal time making labels that do not make sense. * Adding link * Adding the Contributor Covenant Code of Conduct * Update License year Closes shutterstock#589 * Commiting package.lock file * 1.6.2 * update bower spec for build tools: * specify js file extension for main property * pull in rickshaw.css * 1.6.3 * added test for setSeries * chore: updated fields in the package.json - Check that nothing drastic happened in the `package.json`. - We've added " JavaScript toolkit for creating interactive real-time graphs" as the description in the `package.json`. We got this from the GitHub repo description. - Check the `package.json` keywords. We added these from your GitHub topics: "charts", "d3", "graph", "javascript", "rickshaw", "svg". - Check that the bugs field in the package.json is OK. It doesn't match what we'd expect, which would be https://github.com/shutterstock/rickshaw/issues - We expected the repository url in the `package.json` to be https://github.com/shutterstock/rickshaw, and it wasn't. Is this intentional? - If there are more contributors, add them to the Contributors field in the `package.json`. * fixes shutterstock#604 seperate build into minification and development * build before test, fail PR on dist change * Update Table of Contents * 1.6.4 * Fix positioning of hoverdetail in chrome when zooming This commit changes the calculation of the current mouse position to use `MouseEvent.clientX/Y` (mouse position in relation to the viewport) and `Element.getBoundingClientRect()` (element position in relation to the viewport). `event.layerX/Y` is non-standard and seems to return different values than we are expecting if there is zooming involved in Chrome. Demo, zoom in and hover the square: https://jsfiddle.net/6gnpL75j/ An earlier commit reverted the order of layer vs offset and fixed a positioning bug in Firefox while introducing this new one in Chrome: shutterstock@c81f037 I looked around for that Firefox positioning bug in v1.6.0 which I found in: http://code.shutterstock.com/rickshaw/examples/extensions.html With this PR, both Chrome and Firefox show labels in the correct spot. More information: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/clientX https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect https://developer.mozilla.org/en-US/docs/Web/API/UIEvent/layerX Chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=323518 * wrap style access to prevent unnecessary reflow * whitespace * 1.6.5 * fix: check undefined for series opacity * test: add test for opacity settings for scatterplot renderer * Update links * 1.6.6 * not including package-lock since all deps are dev deps * Add auto height and width to detail.css * Bump express from 3.3.5 to 4.17.1 in /examples/socket.io Bumps [express](https://github.com/expressjs/express) from 3.3.5 to 4.17.1. - [Release notes](https://github.com/expressjs/express/releases) - [Changelog](https://github.com/expressjs/express/blob/master/History.md) - [Commits](expressjs/express@3.3.5...4.17.1) Signed-off-by: dependabot[bot] <support@github.com> * Bump clean-css from 3.4.28 to 4.2.1 Bumps [clean-css](https://github.com/jakubpawlowicz/clean-css) from 3.4.28 to 4.2.1. - [Release notes](https://github.com/jakubpawlowicz/clean-css/releases) - [Changelog](https://github.com/jakubpawlowicz/clean-css/blob/master/History.md) - [Commits](clean-css/clean-css@v3.4.28...v4.2.1) Signed-off-by: dependabot[bot] <support@github.com> * add editorconfig * clean-css was split into clean-css-cli * add example of highlight transform for shutterstock#489 * Update Rickshaw.Graph.Axis.Y.js This change allows the user to select the axis color when creating the axis. It is useful when creating axis dynamically (adding series with different scales on run time). * correct formatBase1024KMGTP (shutterstock#620) * correct formatBase1024KMGTP and support negatives values fixes shutterstock#601 * fix indentation * 1.7.0 * check-in screenshots * npm ignore docs tests etc * 1.7.1 * remove local copy of vendor/d3 (shutterstock#613) Removed the local git syncd copy of d3.js since it is already a dependency in both the package.json and bower.json Updated examples to use the copy from node_modules to maintain consistency with the unit tests * docs: Fix typo (shutterstock#630) Co-authored-by: Christopher Chambers <chris@peanutcode.com> Co-authored-by: Robert Buchholz <rbu@goodpoint.de> Co-authored-by: David Chester <david@fmail.co.uk> Co-authored-by: David Chester <dchester@shutterstock.com> Co-authored-by: Martin Häcker <spamfaenger@gmx.de> Co-authored-by: Mark Derbecker <mark.derbecker@seeq.com> Co-authored-by: deldrid1 <deldrid1@utk.edu> Co-authored-by: Noah Gibbs <noah.gibbs@onlive.com> Co-authored-by: Martin Olsson <martin@minimum.se> Co-authored-by: arunv <me@arunv.com> Co-authored-by: Luke Mawbey <lbm@lbm.net.au> Co-authored-by: Katie McLaughlin <katie@anchor.net.au> Co-authored-by: Callum Jones <callum@callumj.com> Co-authored-by: Fernando Jorge Mota <f.j.mota13@gmail.com> Co-authored-by: Shane Reustle <me@shanereustle.com> Co-authored-by: msand <msand@abo.fi> Co-authored-by: Joshua Goldberg <jsgoldberg90@gmail.com> Co-authored-by: Matthew Ginnard <matthew@runscope.com> Co-authored-by: Nicolas Racine <nic335@users.noreply.github.com> Co-authored-by: André Tavares <mail@andretavares.com> Co-authored-by: Brennan Ashton <bashton@brennanashton.com> Co-authored-by: hamx0r <jason.haury@gmail.com> Co-authored-by: Alexander Johansson <alexander@n1s.se> Co-authored-by: GermanBluefox <dogafox@gmail.com> Co-authored-by: Gagan Awhad <gagan.a.awhad@gmail.com> Co-authored-by: Eirik S. Morland <eirik@morland.no> Co-authored-by: Vivek Kushwaha <vivek.kushwaha@hindustantimes.com> Co-authored-by: Sebastian Wallin <sebastian.wallin@gmail.com> Co-authored-by: Michal Ostruszka <michal.ostruszka@gmail.com> Co-authored-by: Michelle Bu <michelle@stripe.com> Co-authored-by: Stuart Nelson <stuartnelson3@gmail.com> Co-authored-by: Mathieu Chataigner <mathieu.chataigner@gmail.com> Co-authored-by: Mike Atlas <mike@weft.io> Co-authored-by: Pablo Reyes <reyesoft@gmail.com> Co-authored-by: funvit <funvit@gmail.com> Co-authored-by: Wil Moore III <wil.moore@wilmoore.com> Co-authored-by: cjthompson <chris@thompson-web.org> Co-authored-by: Jay <jay3686@gmail.com> Co-authored-by: cesine <cesine@yahoo.com> Co-authored-by: cesine <cesine@users.noreply.github.com> Co-authored-by: Thomas Lackemann <tommylackemann@gmail.com> Co-authored-by: Jay Patel <jpatel@shutterstock.com> Co-authored-by: Benjamin J DeLong <howaboutben+github@gmail.com> Co-authored-by: Mathias Rangel Wulff <m@rawu.dk> Co-authored-by: Doa Jafri <doajafri@gmail.com> Co-authored-by: Robert Tod <roberttod@gmail.com> Co-authored-by: Tamas Goga <tamasgoga94@gmail.com> Co-authored-by: Mokeponi <mokeponi@gmail.com> Co-authored-by: bhashit parikh <bhashit.parikh@gmail.com> Co-authored-by: Richard Littauer <richard.littauer@gmail.com> Co-authored-by: Conan Tzou <conan@asperasoft.com> Co-authored-by: Roland Schilter <roli@weave.works> Co-authored-by: Jack Q <qiaobo@outlook.com> Co-authored-by: Diego Zilioti <zilioti.diego@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: ftortega <francisco.tomas.ortega@gmail.com> Co-authored-by: Julien Francoz <jfcoz@users.noreply.github.com> Co-authored-by: Sebastian Murphy <sebastianmurphy@gmail.com> Co-authored-by: Tim Gates <tim.gates@iress.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #337