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.

Sign up for GitHub

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

Jump to bottom

refactor: Migration of Chart to TypeScript #28370

Merged
merged 9 commits into from
Oct 7, 2024
Merged

refactor: Migration of Chart to TypeScript #28370

merged 9 commits into from
Oct 7, 2024

Conversation

EnxDev
Copy link
Contributor

@EnxDev EnxDev commented May 7, 2024

SUMMARY

Migrate Chart to TypeScript

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N.A.

TESTING INSTRUCTIONS

  • All tests should pass

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@john-bodley john-bodley requested a review from justinpark May 7, 2024 17:36
@rusackas rusackas requested a review from kgabryje May 7, 2024 22:10
kgabryje
kgabryje reviewed May 8, 2024
View reviewed changes
superset-frontend/src/components/Chart/Chart.tsx
class Chart extends React.PureComponent<ChartProps, {}> {
static defaultProps = defaultProps;

renderStartTime: any;
Copy link
Member

Choose a reason for hiding this comment

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

Where does renderStartTime come from? In the old version, we're only reading it, but we don't assign any value to it. In your changes you put renderStartTime in ChartProps, but also as class's field. I don't think it's actually passed as a prop anywhere, and as a class field it's also only read, not assigned.
So maybe we should just remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this from ChartProps.
You mean we should remove him from line 198 and 200 ( start_offset and duration keys)?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, unless I misunderstand something here, renderStartTime is always undefined because we don’t set its value anywhere, so maybe we could remove it from the code. @rusackas any objections?

Copy link
Member

Choose a reason for hiding this comment

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

Unless we want to actually fix it and assign a timestamp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think we should understand why it was put there.
I remain waiting for instructions, thank you.

Copy link
Member

Choose a reason for hiding this comment

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

Well, if it's not wired up, that's an odd problem. It sure seems like it'd be useful for logging, which seems to be its intent. There's one in ChartRenderer.jsx that IS wired up, for reference. Maybe we don't need both though?

It WAS set in the original commit, actually. I haven't checked to see when its initialization was removed but clearly there wasn't much outcry.

I'd err on the side of more logging, not less. Maybe @eschutho would be interested in taking more advantage of this insight :)

Copy link
Member

Choose a reason for hiding this comment

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

We can prob remove it and add it back in later if we find that we need it.

kgabryje
kgabryje requested changes May 8, 2024
View reviewed changes
Copy link
Member

@kgabryje kgabryje left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Added a few comments

geido
geido reviewed May 16, 2024
View reviewed changes
superset-frontend/src/components/Chart/Chart.tsx Outdated Show resolved Hide resolved
superset-frontend/src/components/Chart/Chart.tsx Outdated Show resolved Hide resolved
superset-frontend/src/components/Chart/Chart.tsx Outdated Show resolved Hide resolved
superset-frontend/src/components/Chart/Chart.tsx Outdated Show resolved Hide resolved
superset-frontend/src/components/Chart/ChartErrorMessage.tsx Outdated Show resolved Hide resolved
@EnxDev EnxDev requested review from kgabryje and rusackas September 29, 2024 17:28
@geido
Copy link
Member

geido commented Oct 3, 2024

/testenv up

@github-actions github-actions bot added i18n Namespace | Anything related to localization risk:db-migration PRs that require a DB migration size/L api Related to the REST API doc Namespace | Anything related to documentation plugins dependencies:npm github_actions Pull requests that update GitHub Actions code packages i18n:brazilian and removed size/XXL labels Oct 7, 2024
@EnxDev EnxDev force-pushed the refactor/typescript-migration-Chart branch from 1fd6dd6 to 6236bfd Compare October 7, 2024 11:31
@github-actions github-actions bot removed i18n Namespace | Anything related to localization risk:db-migration PRs that require a DB migration api Related to the REST API doc Namespace | Anything related to documentation plugins dependencies:npm github_actions Pull requests that update GitHub Actions code packages i18n:brazilian labels Oct 7, 2024
@geido
Copy link
Member

geido commented Oct 7, 2024

/testenv up

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 7, 2024

@geido Ephemeral environment spinning up at http://54.213.132.35:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@geido geido merged commit 1f01305 into apache:master Oct 7, 2024
34 checks passed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 7, 2024

Ephemeral environment shutdown and build artifacts deleted.

mistercrunch pushed a commit that referenced this pull request Oct 28, 2024
@EnxDev @kgabryje
@justinpark @geido @mistercrunch
refactor: Migration of Chart to TypeScript (#28370)
bbd6a1d 
Co-authored-by: Kamil Gabryjelski <kamil.gabryjelski@gmail.com>
Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
Co-authored-by: Geido <60598000+geido@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geido geido geido left review comments

@eschutho eschutho eschutho left review comments

@justinpark justinpark justinpark left review comments

@kgabryje kgabryje kgabryje approved these changes

@rusackas rusackas Awaiting requested review from rusackas

@villebro villebro Awaiting requested review from villebro

@betodealmeida betodealmeida Awaiting requested review from betodealmeida

@nytai nytai Awaiting requested review from nytai

@mistercrunch mistercrunch Awaiting requested review from mistercrunch

@craig-rueda craig-rueda Awaiting requested review from craig-rueda

@john-bodley john-bodley Awaiting requested review from john-bodley

@dpgaspar dpgaspar Awaiting requested review from dpgaspar

@jinghua-qa jinghua-qa Awaiting requested review from jinghua-qa

@michael-s-molina michael-s-molina Awaiting requested review from michael-s-molina

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@EnxDev @geido @rusackas @justinpark @eschutho @kgabryje