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

add diff labels and orientation support #1295

Merged
merged 5 commits into from
Apr 22, 2019
Merged

Conversation

ashika01
Copy link

@ashika01 ashika01 commented Apr 16, 2019

Fixes: #1286

Improvements on label support for victory-candlestick. This PR includes following label support for candlestick

  • lowLabels
  • lowLabelComponent
  • highLabels
  • highLabelComponent
  • openLabels
  • openLabelComponent
  • closeLabels
  • closeLabelComponent

There is also continued support for labels and labelComponent. style can be applied on all the different labels.

Also support top, bottom, left, right label orientation support for all the labels

TODO:

  • regression testing
  • demo update

Copy link
Contributor

@boygirl boygirl left a comment

Choose a reason for hiding this comment

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

This is looking really good. I'm a little concerned about how getCandleWidth is working, but I need to think more about what has access to active. I would also really like to see more demos / stories

};

const getCandleWidth = (props, style) => {
const { active, datum, data, candleWidth, scale, defaultCandleWidth } = props;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little confusing. You're using the same function that was in Candle and supplying it with the data props that you're calculating for each Candle. That mostly works, but active will not typically be defined at this level. Typically it would get added to state by Victory's event system.

(typeof labelOrientation === "object" && labelOrientation[type]) || labelOrientation;

/* eslint-disable complexity*/
const calculateAxisValues = (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about this function name. I don't think axis is accurate here.

@boygirl
Copy link
Contributor

boygirl commented Apr 18, 2019

@ashika01 I've been trying out this branch, and looking for edge cases. There are a couple issues:

  • tooltips don't look right with some labelOrientations. Here's some openLabel tooltips with labelOrientation: "top".

Screen Shot 2019-04-18 at 10 52 12 AM

I'm also seeing a few console warnings in this situation: Warning: Failed prop type: Invalid prop orientation supplied to VictoryTooltip.

  • It looks like the Box / Border primitive is trying to render elements with unexpected props:

Screen Shot 2019-04-18 at 10 57 57 AM

I was also not able to alter the new labels via events. For example:

events={[{
  target: "data",
  eventHandlers: {
    onMouseOver: () => {
      return [
        {
          target: "openLables",
          mutation: () => {
            return { text: "YO" }
          }
        }
      ];
    }
  }
}]}

I'll look into the event issue if you'd prefer. It looks potentially confusing.

@ashika01
Copy link
Author

ashika01 commented Apr 18, 2019

Last commit include change in box plot as well

@ashika01 ashika01 changed the title [WIP] add diff labels and orientation support add diff labels and orientation support Apr 19, 2019
@boygirl boygirl changed the base branch from master to v33.0.0 April 22, 2019 19:55
@boygirl boygirl merged commit dcfa586 into v33.0.0 Apr 22, 2019
@boygirl boygirl deleted the feature-labelsCandlestick branch April 22, 2019 19:58
@boygirl boygirl mentioned this pull request Aug 22, 2019
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