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

chore: migrate HiddenControl component from jsx to tsx #17315

Merged
merged 4 commits into from
Nov 2, 2021

Conversation

Damans227
Copy link
Contributor

SUMMARY

PR for migrating HiddenControl functional react component from JSX to TSX

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue: [TRACKER] Migrate JavaScript files to TypeScript [TRACKER] Migrate JavaScript files to TypeScript #10004
  • Enhancement (new features, refinement)
  • 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

@Damans227
Copy link
Contributor Author

@etr2460 Ptal, thanks!

…l.tsx

Co-authored-by: Erik Ritter <erik.ritter@airbnb.com>
@etr2460
Copy link
Member

etr2460 commented Nov 1, 2021

@Damans227 looks like there's a couple lint issues here. I'd recommend running npm run type locally to see the typescript errors, and also updating the pr title according to the PR Lint error here

@Damans227
Copy link
Contributor Author

@Damans227 looks like there's a couple lint issues here. I'd recommend running npm run type locally to see the typescript errors, and also updating the pr title according to the PR Lint error here

@etr2460 Looks like there is a type mismatch:

src/explore/components/controls/HiddenControl.tsx:29:31 - error TS2322: Type 'string | number | boolean | Function | any[] | Record<string, any>' is not assignable to type 'string | number | readonly string[] | undefined'.
  Type 'false' is not assignable to type 'string | number | readonly string[] | undefined'.

29   return <Input type="hidden" value={props.value} />;
                                 ~~~~~

  node_modules/@types/react/index.d.ts:2224:9
    2224         value?: string | ReadonlyArray<string> | number | undefined;
                 ~~~~~
    The expected type comes from property 'value' which is declared here on type 'IntrinsicAttributes & Pick<InputProps, "dir" | "form" | "slot" | "style" | "title" | "pattern" | "id" | "children" | "prefixCls" | "className" | ... 281 more ... | "bordered"> & ... 4 more ... & { ...; }'

If I change the type of value in the interface to value: 'string | number | readonly string[] | undefined'; then there are no errors.

@etr2460
Copy link
Member

etr2460 commented Nov 2, 2021

that sounds good to me

@Damans227 Damans227 changed the title migrate HiddenControl component from jsx to tsx chore: migrate HiddenControl component from jsx to tsx Nov 2, 2021
@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #17315 (193a7d3) into master (bea8502) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #17315   +/-   ##
=======================================
  Coverage   77.08%   77.09%           
=======================================
  Files        1037     1037           
  Lines       55647    55644    -3     
  Branches     7608     7609    +1     
=======================================
  Hits        42898    42898           
+ Misses      12499    12496    -3     
  Partials      250      250           
Flag Coverage Δ
javascript 71.26% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../src/explore/components/controls/HiddenControl.tsx 75.00% <100.00%> (ø)
...eFilters/FiltersConfigModal/FiltersConfigModal.tsx 87.71% <0.00%> (+0.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bea8502...193a7d3. Read the comment docs.

@etr2460
Copy link
Member

etr2460 commented Nov 2, 2021

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2021

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

…l.tsx

Co-authored-by: Erik Ritter <erik.ritter@airbnb.com>
@Damans227
Copy link
Contributor Author

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

@etr2460 Looks ok to me.

Screen Shot 2021-11-02 at 1 27 43 PM

@etr2460 etr2460 merged commit 28b494c into apache:master Nov 2, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2021

Ephemeral environment shutdown and build artifacts deleted.

@Damans227 Damans227 deleted the enhancement/migrate-jsx-to-tsx_1 branch November 2, 2021 19:23
AAfghahi pushed a commit that referenced this pull request Jan 10, 2022
* migrate HiddenControl component from jsx to tsx

* Update superset-frontend/src/explore/components/controls/HiddenControl.tsx

Co-authored-by: Erik Ritter <erik.ritter@airbnb.com>

* updating the type of value in the interface

* Update superset-frontend/src/explore/components/controls/HiddenControl.tsx

Co-authored-by: Erik Ritter <erik.ritter@airbnb.com>

Co-authored-by: Erik Ritter <erik.ritter@airbnb.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.5.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 1.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants