superset icon indicating copy to clipboard operation
superset copied to clipboard

chore: Move charts to src/pages folder

Open EugeneTorap opened this issue 2 years ago • 1 comments

SUMMARY

Moved ChartCreation & ChartList to src/pages folder to apply direction outlined in #13632

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

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

EugeneTorap avatar Nov 26 '22 14:11 EugeneTorap

Codecov Report

Merging #22230 (603c8f0) into master (08f45ef) will decrease coverage by 0.03%. The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master   #22230      +/-   ##
==========================================
- Coverage   65.61%   65.57%   -0.04%     
==========================================
  Files        1869     1869              
  Lines       71582    71549      -33     
  Branches     7814     7821       +7     
==========================================
- Hits        46968    46918      -50     
- Misses      22573    22603      +30     
+ Partials     2041     2028      -13     
Flag Coverage Δ
javascript 53.90% <33.33%> (-0.09%) :arrow_down:

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

Impacted Files Coverage Δ
...uperset-frontend/src/pages/ChartCreation/index.tsx 61.11% <ø> (ø)
...uperset-frontend/src/pages/ChartList/ChartCard.tsx 52.17% <ø> (ø)
superset-frontend/src/pages/ChartList/index.tsx 54.61% <ø> (ø)
...set-frontend/src/views/CRUD/welcome/ChartTable.tsx 62.50% <ø> (ø)
superset-frontend/src/views/routes.tsx 55.00% <33.33%> (ø)
superset-frontend/src/setup/setupFormatters.ts 0.00% <0.00%> (-100.00%) :arrow_down:
superset-frontend/src/setup/setupClient.ts 0.00% <0.00%> (-70.00%) :arrow_down:
superset-frontend/src/preamble.ts 0.00% <0.00%> (-44.00%) :arrow_down:
...rontend/src/components/DeprecatedSelect/styles.tsx 52.83% <0.00%> (-28.31%) :arrow_down:
...erset-frontend/src/profile/components/Security.tsx 75.00% <0.00%> (-25.00%) :arrow_down:
... and 74 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Nov 26 '22 15:11 codecov[bot]

I think it looks good, but pinging @michael-s-molina for review as his more familiar with the proposed folder structure.

1 concern though - if we're moving stuff to the new /pages folder, shouldn't we move all pages at once, so that there's no inconsistency before the rest of the pages are moved?

kgabryje avatar Dec 05 '22 11:12 kgabryje

@kgabryje If we move all pages at once we break a lot of open PR. I guess the best strategy would be to move them one by one and select only those pages that have little activity so as not to break current PRs.

EugeneTorap avatar Dec 05 '22 11:12 EugeneTorap

@kgabryje If we move all pages at once we break a lot of open PR. I guess the best strategy would be to move them one by one and select only those pages that have little activity so as not to break current PRs.

I agree. I'm taking the iterative approach to reduce impact.

@EugeneTorap One thing I was planning to do before start moving components to the pages folder was to create a sketch of how it would look in the end. This would help with folder naming definitions, how to break the work into multiple PRs, and with the reviews. This is also a required step for the final wiki page that will contain the final proposed structure. The idea would be to do a before/after of the components involved with the pages so we can validate the structure. Something like:

Before

src
   addSlice
   views
      CRUD
      chart

After

src/pages
   charts
      chartCreation
      chartList   

I think the best place for this work is a Github discussion with the title SIP-61 src/pages structure proposal. Given that you're interested in the project, would you like to draw this sketch and open the discussion?

michael-s-molina avatar Dec 05 '22 13:12 michael-s-molina

@michael-s-molina Deal me in!

EugeneTorap avatar Dec 05 '22 13:12 EugeneTorap

@michael-s-molina Deal me in!

Cool. I'll wait for the discussion before merging this PR then. Feel free to ping me when the discussion is open.

michael-s-molina avatar Dec 05 '22 13:12 michael-s-molina

@michael-s-molina Done! #22330

EugeneTorap avatar Dec 05 '22 19:12 EugeneTorap

@michael-s-molina Can we merge it?

EugeneTorap avatar Jan 12 '23 20:01 EugeneTorap

/testenv up

michael-s-molina avatar Jan 12 '23 20:01 michael-s-molina

@michael-s-molina Ephemeral environment spinning up at http://35.91.199.214:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

github-actions[bot] avatar Jan 12 '23 20:01 github-actions[bot]

Ephemeral environment shutdown and build artifacts deleted.

github-actions[bot] avatar Jan 12 '23 20:01 github-actions[bot]