Skip to content

3d trace types - feature parity #605

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.

Sign up for GitHub

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

Jump to bottom
Merged
dmt0 merged 15 commits into from
Jul 24, 2018
Merged

3d trace types - feature parity #605

dmt0 merged 15 commits into from
Jul 24, 2018

Conversation

@dmt0
Copy link
Contributor

@dmt0 dmt0 commented Jul 23, 2018 •
edited
Loading

General

  • Menu order: Traces, Layout, Axes, Legend, Color Bars, Notes, Shapes, Images, Sliders, Menus
  • Place Violin next to Box

3D scatter

StyleTrace

  • can’t select data source for text
  • on hover - ‘text’ missing
  • Line type broken when multiple traces - can’t get color

Layout

  • aspect ratio missing - manual mode broken in cloud sometimes - setting xyz brings back to auto

Axes

  • Empty sections: Layout

Color Bars

  • Tick color -> Color
  • Borders - Background Color broken - also broken in cloud - plotly returns rgba instead of rgb, when you change it to rgb in json, ColorPicker works "rgba(245, 117, 117, 0)"

Images

  • Relative position to grid missing

3D Line

StyleTrace

  • rename Line Color -> Color

Axes

  • Empty sections: Layout

3D Surface

StyleTrace

  • Colorscale Range missing - auto/custom, min, max

Layout

  • aspect ratio missing

Color Bars

  • rename Vertical padding -> Padding, same for Horizontal

3D Mesh

StyleTrace

  • Show Contours - should hide color and width when “hide”

Layout

  • aspect ratio missing
  • Empty sections: Layout

Color Bars

  • Horizontal Positioning - rename horizontal position -> position

Ternary

  • hover actions - replace flaglist with dorpdown, since you can't deselect both and it behaves funny

Closes #516
Closes #520
Closes #606
Addresses #524

@dmt0 dmt0 self-assigned this Jul 23, 2018
@dmt0 dmt0 requested a review from nicolaskruchten July 23, 2018 20:22
nicolaskruchten
nicolaskruchten suggested changes Jul 23, 2018
View reviewed changes
src/components/fields/VisibilitySelect.js
import Dropdown from './Dropdown';
import nestedProperty from 'plotly.js/src/lib/nested_property';

class UnconnectedVisibilitySelect extends Component {
Copy link
Contributor

@nicolaskruchten nicolaskruchten Jul 23, 2018

Choose a reason for hiding this comment

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

how does this guy handle multiple values ?

Copy link
Contributor Author

@dmt0 dmt0 Jul 23, 2018

Choose a reason for hiding this comment

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

As expected actually - all radios deselected when mixed values, and children hidden. Shows multiple values notes on each field that differs.
So far don't have any dropdown VisibilitySelects in StyleTracePanel - will test that when one appears.

src/default_panels/StyleTracesPanel.js
name={_('Text')}
traceTypes={['scatter', 'scatterpolar', 'scatterpolargl', 'pie']}
traceTypes={[
'scatter',
Copy link
Contributor

@nicolaskruchten nicolaskruchten Jul 23, 2018

Choose a reason for hiding this comment

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

scattergl also? generally they go in pairs :)

Copy link
Contributor Author

@dmt0 dmt0 Jul 23, 2018

Choose a reason for hiding this comment

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

done

@dmt0 dmt0 force-pushed the 3d-fixes branch 2 times, most recently from 34ee91b to 7eb42be Compare July 23, 2018 22:31
nicolaskruchten
nicolaskruchten reviewed Jul 24, 2018
View reviewed changes
src/default_panels/StyleTracesPanel.js
]}
>
<Flaglist
<Dropdown
Copy link
Contributor

@nicolaskruchten nicolaskruchten Jul 24, 2018

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

@dmt0 dmt0 Jul 24, 2018

Choose a reason for hiding this comment

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

Because there are only three options - you cannot deselect both, I found it kinda funny. Subjective really.

This was referenced Jul 24, 2018
Closed
Closed
@nicolaskruchten
Copy link
Contributor

nicolaskruchten commented Jul 24, 2018

Notes:

  • gl3d_bunny-hull.json mock crashes Style/Traces panel
  • (actually many of the gl3d mocks crash that panel)
  • The aspectratio stuff only applies to the first scene in this case (i.e. it will be weird if there are two scenes), so it'll have to move to the new Subplots pane... I'm tempted to remove it in this PR and add it in when we do subplots. IIRC there was some stuff like this when we last looked at polar as well, so when we do the subplots panel we'll have to sweep through and find all the places where we mapped an attr to polar.* or geo.* and move those to the applicable subplot fold I guess :)

@dmt0 dmt0 force-pushed the 3d-fixes branch 3 times, most recently from 6d87e9b to 9d97f8e Compare July 24, 2018 16:22
nicolaskruchten
nicolaskruchten reviewed Jul 24, 2018
View reviewed changes
src/default_panels/StyleTracesPanel.js Outdated
<SymbolSelector label={_('Symbol')} attr="marker.symbol" />
<Numeric label={_('Border Width')} attr="marker.line.width" />
<ColorPicker label={_('Border Color')} attr="marker.line.color" />
<Radio
Copy link
Contributor

@nicolaskruchten nicolaskruchten Jul 24, 2018

Choose a reason for hiding this comment

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

can you plz remove these from line given that we don't allow line colorscales atm :)

nicolaskruchten
nicolaskruchten reviewed Jul 24, 2018
View reviewed changes
src/components/fields/MarkerColor.js
colorscale={colorscale}
/>
)}
<Radio
Copy link
Contributor

@nicolaskruchten nicolaskruchten Jul 24, 2018

Choose a reason for hiding this comment

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

from the Percy screenshot it looks like multi-value is odd with these...

Copy link
Contributor

@nicolaskruchten nicolaskruchten Jul 24, 2018

Choose a reason for hiding this comment

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

well, I could be convinced that the percy screenshot is reasonable, let's talk about it

@nicolaskruchten
Copy link
Contributor

nicolaskruchten commented Jul 24, 2018

💃 ! I think this also closes #520 and #606

@dmt0 dmt0 merged commit 80468be into master Jul 24, 2018
@dmt0 dmt0 deleted the 3d-fixes branch July 24, 2018 18:45
@dmt0 dmt0 mentioned this pull request Aug 9, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nicolaskruchten nicolaskruchten nicolaskruchten approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dmt0 dmt0

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dmt0 @nicolaskruchten