Skip to content

Conversation

dmt0
Copy link
Contributor

@dmt0 dmt0 commented Jul 26, 2018

Subplots panel - in dev

Known issues / TODO:

  • rectangle needs to be positioned correctly - no making it 3px doesn't work
  • DualNumeric is a hack - needs to be done right
  • possibly merge AxesCreator and SubplotCreator - at least partially - lots of ways to improve the code - some utility functions for figuring out subplot names would be useful - raised Refactor: merge AxesCreator and SubplotCreator #631
  • move geo., scene. stuff into subplot panel OR create some SubpotConnectedSections in GraphCreatePanel

addresses #580

@dmt0 dmt0 requested a review from nicolaskruchten July 26, 2018 00:15
@nicolaskruchten
Copy link
Contributor

Dude this is sweet!

@dmt0
Copy link
Contributor Author

dmt0 commented Jul 26, 2018

:)
I'm thinking, shouldn't we just move all that into GraphCreatePanel into the trace folds? The only drawback I see is that if two traces are on the same subplot, their controls will be mirroring each other.

@dmt0 dmt0 force-pushed the subplots branch 13 times, most recently from 0ff1eb6 to 3b728af Compare August 3, 2018 22:44
@nicolaskruchten
Copy link
Contributor

So cool! non-cartesian appears broken, and dragging is broken in snap mode, but so cool!

@dmt0 dmt0 force-pushed the subplots branch 8 times, most recently from 653ff58 to 40c5e1a Compare August 6, 2018 14:53
@dmt0 dmt0 changed the title subplots panel initial - in Dev subplots panel initial Aug 6, 2018
@nicolaskruchten
Copy link
Contributor

In AxesCreator you'll need to update the text to say that you can edit the subplots in the right panel. now it says "axes"

@nicolaskruchten
Copy link
Contributor

image

plotProps.fullContainer._subplot &&
plotProps.fullContainer._subplot.includes('xaxis')
) {
if (props.attr.startsWith('x')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems a little fragile, is there a way to make it more precise?
what if there's other axes related attributes that start with x or y..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call, i'll make it more precise
but generally, this guys get attr supplied to them

import {
GraphCreatePanel,
GraphTransformsPanel,
StyleSubplotsPanel,
Copy link
Contributor

Choose a reason for hiding this comment

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

rename?

<ColorPicker label={_('Font Color')} attr="titlefont.color" />
</AxesFold>

<AxesFold
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah! goodbye Layout Panel! You won't be missed :)

@dmt0 dmt0 force-pushed the subplots branch 2 times, most recently from 041aeec to 90abf62 Compare August 6, 2018 15:34
"react-resizable-rotatable-draggable": "^0.1.8",
"react-select": "^1.2.0",
"react-tabs": "^2.2.1",
"styled-components": "^3.3.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

are we actually using it?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's a peer dep of the draggy thing.

))
);

Object.keys(layout).forEach(layoutKey => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I found it confusing here as to why we were looping through layout, not fullLayout.
And why we were using data.findIndex instead of maybe filter to find all indexes that could meet the condition.

If you could maybe comment above on those 2 questions, I think it may help in a few months time, when revisiting that code.

key={i}
className="rect-grid"
style={{
width: fieldWidthPx / gridRes - 1,
Copy link
Contributor

@VeraZab VeraZab Aug 6, 2018

Choose a reason for hiding this comment

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

could we move this to a style sheet for the ones we can?

Copy link
Contributor

Choose a reason for hiding this comment

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

not really, if you see the math we're doing here, it doesn't really map well onto a stylesheet

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we could create classes for the various grid-box-types but there are 9 of them (4 corners, 4 faces, inside) which would be messier than just building them up from stratch like this IMO. Although with faaaancy CSS3 selectors you might be able to do it ;)

@VeraZab
Copy link
Contributor

VeraZab commented Aug 6, 2018

awesome job guys !! 👍
If @dmt0 you could just leave me a comment in SubplotAccordion of what the json looks like in a situation where we have 2 subplots of either ['geo', 'mapbox', 'polar', 'gl3d', 'ternary'], I'm 💃 on this pr.

@nicolaskruchten
Copy link
Contributor

💃

@dmt0 dmt0 merged commit 975ea1b into master Aug 6, 2018
@dmt0 dmt0 deleted the subplots branch August 6, 2018 17:43
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