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

refactor classes #16

Merged
merged 12 commits into from
Oct 1, 2024
Merged

Conversation

dfguerrerom
Copy link
Contributor

@dfguerrerom dfguerrerom commented Sep 30, 2024

@trungleduc sorry for the mess... I was expecting first merge this in my fork, yesterday I think both of us were working in the same thing.

In this PR I have used yours as base and have done some editions.

  • Create a single initEcharts which will setOptions based on the _createOptionDict.
  • I also set the option to listen changes in the rawWidget , what do you think?
  • I added the theme as another parameter, it will be used only if users set this parameter in the charts or if they modify it directly, otherwise the themeManager will be used as before.
  • I haven't fully tested the other properties
Screencast.from.30-09-2024.12.45.33.webm

This is a WIP

@trungleduc
Copy link
Owner

Hey, thank you for opening the PR. Maybe it is a duplication with #14 ?

@trungleduc
Copy link
Owner

Let me merge #14 then you can rebase your PR on top of it?

@dfguerrerom
Copy link
Contributor Author

Hey, thank you for opening the PR. Maybe it is a duplication with #14 ?

hey, sorry, I was supposed to test it first in my branch but I selected the wrong base branch....
Actually this extends #14... the PR is done on that branch...

@trungleduc
Copy link
Owner

Merged, let's target main now

@dfguerrerom dfguerrerom changed the base branch from refactor-classes to main September 30, 2024 10:36
@dfguerrerom dfguerrerom marked this pull request as draft September 30, 2024 10:49
@trungleduc
Copy link
Owner

No problem at all. I will add support for events and actions in a few days

@dfguerrerom
Copy link
Contributor Author

@trungleduc I have converted this PR as a draft, I think something went wrong with the merge... let me figure it out and I'll ask your review!

@dfguerrerom
Copy link
Contributor Author

In my last commits I have also added the height and width options... If users set those properties, the default css classes won't be applied, otherwise the default will be used, what do you think?

  • I couldn't see the effect of 'useDirtyRect`, do you know if there's a reason for that?
  • I'm not very sure about how to define the default values for the properties in the baseWidgetModel, is it fine just let them as null in both sides?

@dfguerrerom dfguerrerom marked this pull request as ready for review September 30, 2024 13:57
@trungleduc trungleduc added the enhancement New feature or request label Sep 30, 2024
src/baseWidgetView.ts Outdated Show resolved Hide resolved
@trungleduc
Copy link
Owner

Thanks! it looks neat. I will add example and documentation later

@dfguerrerom
Copy link
Contributor Author

I have tested the trait changes and they work well... However, there's an error when the 'svg' renderer is set and width/height are not a number... I think it is harmless because the class applied to the element will be set in the end, what do you think?

image

@trungleduc
Copy link
Owner

I have tested the trait changes and they work well... However, there's an error when the 'svg' renderer is set and width/height are not a number... I think it is harmless because the class applied to the element will be set in the end, what do you think?

image

The chart size is still correct?

@dfguerrerom
Copy link
Contributor Author

I have tested the trait changes and they work well... However, there's an error when the 'svg' renderer is set and width/height are not a number... I think it is harmless because the class applied to the element will be set in the end, what do you think?
image

The chart size is still correct?

if you pass auto/null to height/width, the default .css rule is applied, and therefore, the chart is displayed correctly.

If you pass a valid pixel value, i.e., '200', '200px', it removes the default class and instead, it correctly passes those properties to the init and there's no error.

@trungleduc
Copy link
Owner

ok so it's good for me

@trungleduc
Copy link
Owner

Thanks!

@trungleduc trungleduc merged commit cdf4c61 into trungleduc:main Oct 1, 2024
8 checks passed
@dfguerrerom dfguerrerom deleted the refactor-classes branch October 1, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants