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

feat(funnel): funnel chart support orient API #12754

Merged
merged 5 commits into from
Aug 5, 2020

Conversation

regrex
Copy link
Contributor

@regrex regrex commented Jun 4, 2020

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

Funnel chart support orient API with value vertical(default) or horizontal.

Fixed issues

Details

Usage

Are there any API changes?

  • The API has been changed.

series-funnel.orient: vertical | horizontal

Related test cases or examples to use the new APIs

NA.

option = {
  series: [
      {
          name:'漏斗图',
          type:'funnel',
          gap: 3,
          left: 300,
          right: 300,
          orient: 'horizontal', // vertical | horizontal, default value is vertical
          label: {
              normal: {
                  position: 'leftTop'
              }
          },
          data:[
              {value:60, name:'访问'},
              {value:40, name:'咨询'},
              {value:20, name:'订单'},
              {value:80, name:'点击'},
          ]
      }
  ]
}

Others

Merging options

  • Please squash the commits into a single one when merge.

Other information

@Ovilia Ovilia added this to the 4.9.0 milestone Jun 4, 2020
@regrex regrex changed the title Funnel chart support orient API feat: funnel chart support orient API Jun 8, 2020
@regrex regrex changed the title feat: funnel chart support orient API feat(funnel): funnel chart support orient API Jun 8, 2020
@100pah
Copy link
Member

100pah commented Jun 9, 2020

[DISCUSS_layout_config] @pissang @Ovilia @regrex

If intending to support layout: 'horizontal' in funnel,
Is it OK for these option design?


sort: 'ascending', // porp "sort" exists for long time.
orient: 'vertical',

image


sort: 'descending', 
orient: 'vertical',

image


sort: 'asscending', 
orient: 'horizontal',

image


sort: 'descending', 
orient: 'horizontal',

image

@100pah
Copy link
Member

100pah commented Jun 9, 2020

[DISCUSS_label_position_config] @pissang @Ovilia @regrex

If there [DISCUSS_layout_config] above is OK, what about the label position config?

Currently, for vertical layout, funnel label.position supports
'insdie'/'insideLeft'/'insideRight'/'left'/'right'/'leftTop'/'leftBottom'/'rightTop'/'rightBottom'.

But

Question 1:

There are counterintuitive behavior for 'leftTop'/'leftBottom'/'rightTop'/'rightBottom' in currently layout: 'vertical', sort: 'ascending', (might be a existing bug):

orient: 'vertical', 
sort: 'ascending'
label: {
    position: 'rightTop'
}

But actually the labels are placed at the position "rightBottom":
image

And

orient: 'vertical', 
sort: 'ascending'
label: {
    position: 'rightBottom'
}

actually the labels are placed at the position "rightTop":
image

The reason probably be: the implementation of label position is only according to the case of
sort: 'descending'. So if set to sort: 'ascending', the behavior is counterintuitive and seams do not follow the convention of echarts option.

Thus the question is: should we correct it?
If we correct it, that would be a break change. Is it worth to do that?


Question 2:

Now we need to support orient: 'vertical', the same problem occurs:
how would we design the meaning of label.position values left/right/leftTop/leftBottom ... ?

(A) If we follow the "counterintuitive" design: the position should be only understand on the case of sort: 'descending', orient: 'vertical' and "rotate in mind" to the final effect.

It is counterintuitive:

orient: 'right',
label: { position: 'right' } // use position right to place label on bottom

image

But that would be simple in implementation and do not need to worry about break change (described in question 1). In a sense, it can be reasonable since funnel is literally something always "vertical" in mind:
image

(B) Make it totally follow the intuitive:
label.postion: 'top' means label is top.
label.postion: 'rightTop' means label is right top.
But it might have break change, and bring little complex in code.

So the question is: (A) or (B) ?

@regrex
Copy link
Contributor Author

regrex commented Jun 10, 2020

@100pah I have also considered this problem. At the beginning, my plan is to increase the value of position for the original scenario of horizontal: top \ bottom \ topleft \ topright \ bottomleft \ bottomright. But later, considering that users need to associate two APIs to view documents at the same time, it simplifies the design of position. When the original is horizontal, I think it is to connect the entire entire funnel Chart rotates 90 degrees anticlockwise, and the left value of position corresponds to the bottom value of horizontal. At present, my mapping relationship implementation is a bit messy, which needs to be adjusted.

@regrex regrex force-pushed the feat-funnel-orient branch from 6e5beb7 to a12b9c7 Compare June 10, 2020 07:36
@@ -86,9 +86,10 @@
left: 300,
right: 300,
sort: 'ascending',
orient: 'horizontal',
Copy link
Member

Choose a reason for hiding this comment

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

We should better add some new test cases for orient: 'horizontal' rather than change the original 'vertical' test cases.
Hint: in echarts source dir,

npm run mktest funnel2

will generate a test caes file test/funnel2.html

@100pah 100pah merged commit 450ee94 into apache:master Aug 5, 2020
@echarts-bot
Copy link

echarts-bot bot commented Aug 5, 2020

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

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