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: Move charts to src/pages folder #22230

Merged
merged 7 commits into from
Jan 12, 2023
Merged

chore: Move charts to src/pages folder #22230

merged 7 commits into from
Jan 12, 2023

Conversation

EugeneTorap
Copy link
Contributor

@EugeneTorap EugeneTorap commented Nov 26, 2022

SUMMARY

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

This work is part of SIP-61

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

@codecov
Copy link

codecov bot commented Nov 26, 2022

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%) ⬇️

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%) ⬇️
superset-frontend/src/setup/setupClient.ts 0.00% <0.00%> (-70.00%) ⬇️
superset-frontend/src/preamble.ts 0.00% <0.00%> (-44.00%) ⬇️
...rontend/src/components/DeprecatedSelect/styles.tsx 52.83% <0.00%> (-28.31%) ⬇️
...erset-frontend/src/profile/components/Security.tsx 75.00% <0.00%> (-25.00%) ⬇️
... and 74 more

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

@kgabryje
Copy link
Member

kgabryje commented Dec 5, 2022

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?

@EugeneTorap
Copy link
Contributor Author

@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.

@michael-s-molina
Copy link
Member

@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?

@EugeneTorap
Copy link
Contributor Author

@michael-s-molina Deal me in!

@michael-s-molina
Copy link
Member

@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.

@EugeneTorap
Copy link
Contributor Author

@michael-s-molina Done! #22330

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @EugeneTorap. Some comments:

  • It seems types.ts is not being used anywhere. If that's the case, we can delete it.
  • ChartCard is used by ChartList and Welcome pages. It should belong in the features/charts folder.
  • ChartCreation should be renamed to index.tsx to follow the same pattern used by the components folder

@EugeneTorap
Copy link
Contributor Author

@michael-s-molina Can we merge it?

@michael-s-molina
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@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.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM

@michael-s-molina michael-s-molina merged commit 1a0de49 into apache:master Jan 12, 2023
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@EugeneTorap EugeneTorap deleted the chore/move-charts-to-pages branch January 12, 2023 21:19
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 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/M 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants