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

Scatter Plot Panel #63

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Scatter Plot Panel #63

wants to merge 14 commits into from

Conversation

timio23
Copy link
Contributor

@timio23 timio23 commented Sep 21, 2024

No description provided.

@formfcw formfcw self-requested a review October 9, 2024 05:12
Copy link
Contributor

@formfcw formfcw left a comment

Choose a reason for hiding this comment

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

Great work @timio23 🎉

In addition to the comments in the code, here is a list of things I noticed while visually reviewing the extension. Please be sure to update the code to at least fix the bugs!

  1. Decimal values do not affect the X-axis

    screenshot

    bug-decimal-values

  2. The dots do not align with the x-axis label when the field is a datetime or timestamp field

    screenshot

    datetime-int-dot-alignment

  3. Currently there are two tooltips, we could get rid of the bottom one

    screenshot

    double-tooltip

  4. If you hover over a dot, it gets bigger and its opacity is reduced; if tooltips are disabled, it just reduces the opacity without getting bigger. Make sure you are consistent here. I would suggest just scaling the dot up a bit on hover or changing the opacity to 100%.

    screenshots

    hover-dot-tooltip
    hover-dot

  5. When you click on a dot, it gets a gray border, which seems unintentional. You could set its opacity to 100% and remove the border (or make it white) instead.

  6. You could change the format of the tooltips from …

    `{{ valueOfXAxis }}`
    `{{ colorDot }} {{ collectionName }}: {{ valueOfYAxis }}`
    // or with Series Grouping enabled
    `{{ colorDot }} {{ seriesGroupName }}: {{ valueOfYAxis }}`

    … to …

    `{{ fieldNameOfXAxis }}: {{ valueOfXAxis }}`
    `{{ colorDot }} {{ fieldNameOfYAxis }}: {{ valueOfYAxis }}`
    // or with Series Grouping enabled
    `{{ colorDot }} {{ seriesGroupName }} – {{ fieldNameOfYAxis }}: {{ valueOfYAxis }}`

packages/scatter-plot-panel/package.json Outdated Show resolved Hide resolved
packages/scatter-plot-panel/src/panel.vue Outdated Show resolved Hide resolved
packages/scatter-plot-panel/package.json Show resolved Hide resolved
packages/scatter-plot-panel/src/panel.vue Outdated Show resolved Hide resolved
packages/scatter-plot-panel/src/index.ts Show resolved Hide resolved
packages/scatter-plot-panel/src/panel.vue Outdated Show resolved Hide resolved
packages/scatter-plot-panel/README.md Outdated Show resolved Hide resolved
packages/scatter-plot-panel/README.md Outdated Show resolved Hide resolved
packages/scatter-plot-panel/README.md Outdated Show resolved Hide resolved
@BB-Loft
Copy link
Contributor

BB-Loft commented Oct 11, 2024

Hey @timio23 - can you please let me know your plans and availability for reviewing these changes? Thanks.

@timio23
Copy link
Contributor Author

timio23 commented Oct 11, 2024

Hi @BB-Loft, I’ve been overseas this week and just got home. I will look at this tonight

@timio23
Copy link
Contributor Author

timio23 commented Oct 12, 2024

Thanks for your comments @formfcw, I've addressed most of the comments.

  1. Wasn't able to reproduce this. X Axis working perfectly for me as integer, decimal and float. Could you provide more information on how you reached this result?
  2. The X axis for datetime values are factoring the time when plotting. If I set the time points to exactly midnight, the dots line up perfectly. This should be more visible with the recent changes to the tooltips. (It seems the Directus datetime picker defaults to midday)
  3. Bottom tooltip removed as requested
  4. Dots now remain the same size. Opacity is the only change
  5. Yes, this was unintentional. Stroke has now been removed when a dot is clicked.
  6. Tooltip formatting was a little strict when I keep the automations in place. I went for a similar approach along the lines of what you suggested.
`{{ fieldNameOfXAxis }}: {{ valueOfXAxis }}`
`{{ colorDot }} {{ collectionName }}: {{ fieldNameOfYAxis }} {{ valueOfYAxis }}`
// Series Grouping enabled
`{{ colorDot }} {{ seriesGroupName }}: {{ fieldNameOfYAxis }} {{ valueOfYAxis }}`

Changes to dependencies, readme and index have been made as suggested.

Copy link
Contributor

@formfcw formfcw left a comment

Choose a reason for hiding this comment

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

Thanks @timio23! A few things …

> 1. Wasn't able to reproduce this. X Axis working perfectly for me as integer, decimal and float. Could you provide more information on how you reached this result?

I am still running into this problem. I'm using MySQL - maybe that's the reason. Can you test?

(Update: see below)


  1. The X axis for datetime values are factoring the time when plotting. If I set the time points to exactly midnight, the dots line up perfectly. This should be more visible with the recent changes to the tooltips. (It seems the Directus datetime picker defaults to midday)

You are right with that the date picker defaults to midday, at the same time it still doesn’t align well, even if I set the value to 12 AM (00:00).

packages/scatter-plot-panel/package.json Outdated Show resolved Hide resolved
packages/scatter-plot-panel/src/panel.vue Outdated Show resolved Hide resolved
packages/scatter-plot-panel/src/panel.vue Show resolved Hide resolved
@formfcw
Copy link
Contributor

formfcw commented Oct 12, 2024

OK, @timio23. Please make sure that decimal values (for x-axis) work at least with sqlite3 and Postgres as db.

@timio23
Copy link
Contributor Author

timio23 commented Oct 14, 2024

@formfcw, the datetime alignment seems to be a UTC issue. I found a parameter that disables UTC on the x-axis so it uses your devices local time instead. This should align to your chosen timepoints now. As for the decimals and floats, I've forced the value to a string in the hope it will standardize the data sent to Apex charts. Theoretically Knex should be returning the data in the same way regardless of the database.

Copy link
Contributor

@formfcw formfcw left a comment

Choose a reason for hiding this comment

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

Thank you @timio23. I have tested your panel with postgres db and here I have the same problem with decimal values on the x-axis. All the dots align to zero and when I hover over a dot I get a console error. Please make sure it works.

@timio23
Copy link
Contributor Author

timio23 commented Oct 15, 2024

@formfcw, I managed to replicate the issue. The api returns the floats and decimals as strings instead of numbers. Forced the strings to numbers by using * 1. Does this work for you as well?

Copy link
Contributor

@formfcw formfcw left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Thank you!

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.

4 participants