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

add pane primitives #1660

Merged
merged 15 commits into from
Aug 13, 2024
Merged

add pane primitives #1660

merged 15 commits into from
Aug 13, 2024

Conversation

SlicedSilver
Copy link
Contributor

@SlicedSilver SlicedSilver commented Jul 29, 2024

Type of PR: enhancement

Update: INCLUDES the Watermark Plugin changes as well.

PR checklist:

  • Addresses an existing issue: fixes #
  • Includes tests
  • Documentation update

Overview of change:
Adds Pane Primitives which are attached to a pane instead of requiring a series.

To do

  • Update documentation guide for Plugins
  • Fix interaction test for hit test
  • remove attribution and background from graphics e2e test

Copy link

github-actions bot commented Jul 29, 2024

size-limit report 📦

Path Size
ESM 43.02 KB (+1.41% 🔺)
ESM createChart 41.8 KB (-1.02% 🔽)
ESM createChartEx 40.55 KB (-0.83% 🔽)
Standalone-ESM 0 B (-100% 🔽)
Standalone 44.45 KB (+1.34% 🔺)
ESM Standalone 44.49 KB (+100% 🔺)
Plugin: Text Watermark 1.79 KB (+100% 🔺)
Plugin: Image Watermark 1.61 KB (+100% 🔺)

@SlicedSilver SlicedSilver marked this pull request as ready for review August 8, 2024 09:24
@SlicedSilver SlicedSilver requested a review from illetid August 8, 2024 09:34
@illetid
Copy link
Contributor

illetid commented Aug 8, 2024

Screen.Recording.2024-08-08.at.13.28.12.mov

when I'm attaching the watermark

const imageWatermark = new LightweightCharts.ImageWatermark(imageDataUrl, {
	alpha: 0.5,
	padding: 20,
    maxHeight: 60
});

and then resize the pane, the watermark stay at the same position and if I make a pane small enough watermark is not visible completely.
upd: if we resize chart completely with chart.resize position is recalculated

@SlicedSilver
Copy link
Contributor Author

Screen.Recording.2024-08-08.at.13.28.12.mov
when I'm attaching the watermark

const imageWatermark = new LightweightCharts.ImageWatermark(imageDataUrl, {
	alpha: 0.5,
	padding: 20,
    maxHeight: 60
});

and then resize the pane, the watermark stay at the same position and if I make a pane small enough watermark is not visible completely. upd: if we resize chart completely with chart.resize position is recalculated

Yeah, that makes sense that is broken. The code is written to do the positioning calculations on the view (assuming there is only one pane). I need to move that placement position logic into the renderer (which has access to the actual pane canvas dimensions). The plugin was written before we had multiple pane support and this wasn't considered.

I'll fix this. Thanks for spotting it.

@SlicedSilver SlicedSilver requested a review from illetid August 9, 2024 08:47
Copy link
Contributor

@illetid illetid left a comment

Choose a reason for hiding this comment

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

LGTM. great job and really nice improvement for plugins capabilities 🚀

src/model/chart-model.ts Outdated Show resolved Hide resolved
@illetid
Copy link
Contributor

illetid commented Aug 9, 2024

@SlicedSilver one minor thing, could you pls check the failing panes/remove-pane graphic test which is failing, I tried locally and it works for me, but better to double-check

@SlicedSilver
Copy link
Contributor Author

@SlicedSilver one minor thing, could you pls check the failing panes/remove-pane graphic test which is failing, I tried locally and it works for me, but better to double-check

@illetid
I've changed the code for the destroying of the pane. Could you please have a look and check?

@SlicedSilver SlicedSilver merged commit a20f084 into v5-candidate Aug 13, 2024
22 of 33 checks passed
@SlicedSilver SlicedSilver deleted the pane-primitives branch August 13, 2024 08:42
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.

2 participants