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

dash/20756-edit-mode-change-event #21100

Merged
merged 22 commits into from
Jun 3, 2024

Conversation

ddragula
Copy link
Member

@ddragula ddragula commented Apr 30, 2024

Added edit mode events. Changed the way components are updated in edit mode. See #20756.


Closes #20756

TO DO:

  • Change the moment of component update in the edit mode accordion.
  • Add edit-mode events called when updating component options.
  • Add edit-mode events called when editing the layout.
  • Add a modal popup confirming whether to cancel component editing when clicked out of the accordion.
  • Add regression tests.
  • Update docs.

@ddragula ddragula self-assigned this Apr 30, 2024
@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Apr 30, 2024

File size comparison

Sizes for compiled+gzipped (bold) and compiled files.

master candidate difference
dashboards/dashboards.js 43.2 kB
148.6 kB
43.2 kB
148.7 kB
17 B
67 B
dashboards/modules/layout.js 11.7 kB
45.9 kB
12.3 kB
48.4 kB
553 B
2532 B

@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Apr 30, 2024

Lighthouse report

dashboards-demo-minimal.json

Reference Proposed Diff
performance – score 1 1 0.00
first-contentful-paint – score 1 1 0.00
first-contentful-paint – milliseconds 220.89 187.91 -32.98
first-meaningful-paint – score 1 1 0.00
first-meaningful-paint – milliseconds 453.85 433.59 -20.26
dom-size – score 1 1 0.00
dom-size – elements 344 344 0.00

@highsoft-bot
Copy link
Collaborator

Visual test results - No difference found

@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Apr 30, 2024

Dashboard visual diffs

No differences found

@highsoft-bot
Copy link
Collaborator

highsoft-bot commented Apr 30, 2024

Benchmark report - Dashboards

benchmarks/Dashboards/DataPool-CSV-constructor.bench.ts

Sample size This PR avg (ms) master avg (ms) Diff Percent diff
2500000 0.17 0.17 0 -2%
See all
Sample size This PR avg (ms) master avg (ms) Diff Percent diff
100 0.16 0.16 0 0%
1000 0.17 0.17 0 0%
10000 0.16 0.17 -0.01 -4%
100000 0.17 0.16 0 3%
1000000 0.17 0.16 0.01 3%
2500000 0.17 0.17 0 -2%

benchmarks/Dashboards/DataTable-loading-columns.bench.ts

Sample size This PR avg (ms) master avg (ms) Diff Percent diff
2500000 16.51 16.63 -0.12 -1%
See all
Sample size This PR avg (ms) master avg (ms) Diff Percent diff
100 0.25 0.24 0.01 4%
1000 0.24 0.24 0 0%
10000 0.44 0.28 0.16 57.99999999999999%
100000 2.34 2.31 0.03 1%
1000000 9.41 9.15 0.26 3%
2500000 16.51 16.63 -0.12 -1%

benchmarks/Dashboards/DataTable-loading-rows.bench.ts

Sample size This PR avg (ms) master avg (ms) Diff Percent diff
2500000 743.86 743.14 0.72 0%
See all
Sample size This PR avg (ms) master avg (ms) Diff Percent diff
100 0.29 0.3 -0.01 -2%
1000 0.51 0.53 -0.02 -4%
10000 3.18 2.9 0.28 10%
100000 15.9 20.64 -4.74 -23%
1000000 289.4 290.31 -0.92 0%
2500000 743.86 743.14 0.72 0%

@ddragula ddragula added Changelog: Feature Use on PR to add description as a feature in the generated changelog. Product: Highcharts Dashboards labels May 10, 2024
@ddragula ddragula marked this pull request as ready for review May 10, 2024 14:48
@ddragula ddragula requested a review from jomar-honsi May 10, 2024 14:49
Copy link
Contributor

@karolkolodziej karolkolodziej left a comment

Choose a reason for hiding this comment

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

  1. Maskiis missing steps: edit component -> click outside of the sidebar -> click cancel
    2. IMO clicking outside of the dashboard's container should not trigger the popup

css/dashboards/dashboards.css Outdated Show resolved Hide resolved
docs/dashboards/edit-mode.md Outdated Show resolved Hide resolved
samples/dashboards/edit-mode/events/demo.js Show resolved Hide resolved
test/cypress/dashboards/integration/EditMode/sidebar.cy.js Outdated Show resolved Hide resolved
ts/Dashboards/EditMode/AccordionMenu.ts Show resolved Hide resolved
ts/Dashboards/EditMode/AccordionMenu.ts Show resolved Hide resolved
Copy link

@stitot stitot left a comment

Choose a reason for hiding this comment

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

Nice work! Are docs unclear on the fact that not all changes will trigger events, such as component (not chart) options such as title? Possible to add support for these as well?

PS. Really nice(!) that components are updated when fields are unfocused!

@ddragula
Copy link
Member Author

ddragula commented Jun 3, 2024

Are docs unclear on the fact that not all changes will trigger events, such as component (not chart) options such as title? Possible to add support for these as well?

In what case did you observe this, @stitot? My point was that events are always emitted when confirming or cancelling changes, regardless of the option type. I can't reproduce this, it seems to work here too.
image

@stitot
Copy link

stitot commented Jun 3, 2024

Hm, it work fine now, @vazonik . When I tried last time only chart options, not component, triggered the event. All good!

@sebastianbochan sebastianbochan merged commit 31bc774 into master Jun 3, 2024
17 checks passed
@ddragula ddragula deleted the dash/20756-edit-mode-change-event branch June 3, 2024 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Feature Use on PR to add description as a feature in the generated changelog. Product: Highcharts Dashboards
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard layout and configuration change event
6 participants