Skip to content

Conversation

@sroy3
Copy link
Contributor

@sroy3 sroy3 commented Aug 24, 2022

Closes #2248

Screen.Recording.2022-08-24.at.5.02.01.PM.mov

Try it yourself: https://5ffc93f46e9344002155f496-hvagdnhbph.chromatic.com/?path=/story/plots--smooth-template

@sroy3 sroy3 added the bug Something isn't working label Aug 24, 2022
@sroy3 sroy3 self-assigned this Aug 24, 2022
e.stopPropagation()
}

for (const panel of Object.values(panels)) {

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

}

return () => {
for (const panel of Object.values(panels)) {

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

@shcheklein
Copy link
Contributor

Good fix, thanks @sroy3 . Can we control its color / position? Now it looks foreign a bit (clearly let's merge and ship this first).

@sroy3
Copy link
Contributor Author

sroy3 commented Aug 25, 2022

Good fix, thanks @sroy3 . Can we control its color / position? Now it looks foreign a bit (clearly let's merge and ship this first).

Color, it's possible. I simply added a background, but did not try changing the default of the slider. Position is a little harder. Adding the slider into the plot rectangle makes the plot really small. Like you mentioned, I was thinking we should iterate on this after merging the fix.

@sroy3 sroy3 marked this pull request as ready for review August 25, 2022 15:19
@sroy3
Copy link
Contributor Author

sroy3 commented Aug 25, 2022

Screen Shot 2022-08-25 at 2 15 39 PM

With theme accent color

@sroy3
Copy link
Contributor Author

sroy3 commented Aug 25, 2022

Do not merge yet. Seems like there is a empty panel that pops up sometimes on other plots Fixed

panel.removeEventListener('click', disableClick)
panel.addEventListener('mouseenter', addDisabled)
panel.addEventListener('mouseleave', removeDisabled)
panel.addEventListener('click', disableClick)
Copy link
Contributor

Choose a reason for hiding this comment

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

[Q] What is the reason for recreating these?

Copy link
Contributor Author

@sroy3 sroy3 Aug 25, 2022

Choose a reason for hiding this comment

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

Since vega-bindings is being created by vega and that the DragDropContainer rebuilds the plots, setting these events on re-render is the only way to hook on them. Doing so once would create them too early. Maybe I can set up an observer or something to add the listeners once the elements are ready. I'll look into it and do a follow up if possible.

@sroy3 sroy3 enabled auto-merge (squash) August 25, 2022 22:26
@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit bd48f10 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.7% (0.0% change).

View more on Code Climate.

@sroy3 sroy3 merged commit 5b4593b into main Aug 25, 2022
@sroy3 sroy3 deleted the smooth-plots branch August 25, 2022 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue with plots: template: smooth

3 participants