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.

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

feat: udpate MosaicViewProperties #20759

Merged
merged 2 commits into from
Feb 17, 2021
Merged

feat: udpate MosaicViewProperties #20759

merged 2 commits into from
Feb 17, 2021

Conversation

TCL735
Copy link
Contributor

@TCL735 TCL735 commented Feb 16, 2021

Closes influxdata/ui#510

Updates the Mosaic View Properties to include hover dimension and two new properties for the y-axis. This fleshes out Mosaic graphs to render properly any data set. Also improves the hovering.

  • Well-formatted commit messages
  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Feature flagged (if modified API)

@TCL735 TCL735 requested a review from a team as a code owner February 16, 2021 02:07
@TCL735 TCL735 mentioned this pull request Feb 16, 2021
Closed
6 tasks
danxmoran
danxmoran reviewed Feb 16, 2021
View reviewed changes
Copy link
Contributor

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

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

(Caveat: I'm not as familiar with this area of the code)

My main concern is around backwards-compatibility with existing models that don't specify these fields.

Also: could you add a line to CHANGELOG.md?

pkger/clone_resource.go
ch.XCol = p.XColumn
ch.GenerateXAxisTicks = p.GenerateXAxisTicks
ch.XTotalTicks = p.XTotalTicks
ch.XTickStart = p.XTickStart
ch.XTickStep = p.XTickStep
ch.YLabelColumnSeparator = p.YLabelColumnSeparator
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do anything special here if YLabelColumnSeparator is empty, for backwards-compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need. Empty string will be interpreted correctly by the UI.

pkger/clone_resource.go
ch.XCol = p.XColumn
ch.GenerateXAxisTicks = p.GenerateXAxisTicks
ch.XTotalTicks = p.XTotalTicks
ch.XTickStart = p.XTickStart
ch.XTickStep = p.XTickStep
ch.YLabelColumnSeparator = p.YLabelColumnSeparator
ch.YLabelColumns = p.YLabelColumns
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question, should we do anything special for empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Empty array will be interpreted correctly by the UI.

http/swagger.yml
@@ -9271,6 +9277,9 @@ components:
type: string
ySuffix:
type: string
hoverDimension:
type: string
enum: [auto, x, y, xy]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mark one of these as the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Empty string will be interpreted correctly by the UI.

pkger/clone_resource.go
@@ -832,11 +832,14 @@ func convertCellView(cell influxdb.Cell) chart {
ch.Kind = chartKindMosaic
ch.Queries = convertQueries(p.Queries)
ch.Colors = stringsToColors(p.ViewColors)
ch.HoverDimension = p.HoverDimension
Copy link
Contributor

Choose a reason for hiding this comment

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

And here, should we handle empty / unset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need.

pkger/parser_models.go
XColumn: c.XCol,
GenerateXAxisTicks: c.GenerateXAxisTicks,
XTotalTicks: c.XTotalTicks,
XTickStart: c.XTickStart,
XTickStep: c.XTickStep,
YLabelColumnSeparator: c.YLabelColumnSeparator,
YLabelColumns: c.YLabelColumns,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same questions about handling empty / unset for these new fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need. They will be interpreted correctly.

pkger/testdata/dashboard_mosaic.yml
- foo
ySeriesColumns:
- _value
- foo
Copy link
Contributor

Choose a reason for hiding this comment

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

If you leave the test and this file unchanged, does the existing test logic still pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@TCL735
Copy link
Contributor Author

TCL735 commented Feb 16, 2021

Changelog updated.

@TCL735
Copy link
Contributor Author

TCL735 commented Feb 17, 2021

The latest force push was to rebase on top of master.

@TCL735 TCL735 merged commit 084abc3 into master Feb 17, 2021
@TCL735 TCL735 deleted the feat_510_mosaic_interface branch February 17, 2021 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danxmoran danxmoran danxmoran approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mosaic: y-axis column selections
2 participants
@TCL735 @danxmoran