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

Remove unused components from VZE #1483

Merged
merged 19 commits into from
Aug 1, 2024

Conversation

roseeichelmann
Copy link
Contributor

@roseeichelmann roseeichelmann commented Jul 3, 2024

Associated issues

Closes cityofaustin/atd-data-tech#17392

Ended up finding a lot more unused components than just the Charts

Testing

URL to test:
Locally

Steps to test:

  1. Check out this branch and start your data model DB
  2. Check around the app to make sure nothing is broken
  3. Admire all the red 😍

Ship list

  • Check migrations for any conflicts with latest migrations in master branch
  • Confirm Hasura role permissions for necessary access
  • Code reviewed
  • Product manager approved

@johnclary
Copy link
Member

@roseeichelmann would you mind pointing this PR against data-model-v2 instead of master?

Copy link
Member

@johnclary johnclary left a comment

Choose a reason for hiding this comment

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

I tested this locally after merging it with the data model collector branch—all looks great!

I guess, fwiw, it might actually be helpful to preserve all of this for a future product manager who wanted to browse all the UI components, but I would rather see a tool like this hosted in a separate fork or something. As it is, this feature bogs down the code base and I'm very happy to see it removed.

It looks like there are a few more files/routes that can be deleted, then this will be good to go! 🙌

@@ -71,70 +42,6 @@ const routes = roles => [
{ path: "/dev/theme", exact: true, name: "Theme", component: Colors },
Copy link
Member

Choose a reason for hiding this comment

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

any reason not to get rid of these other pieces? Notifications, Theme, Dev...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doneee

@roseeichelmann roseeichelmann changed the base branch from master to data-model-v2 July 8, 2024 17:08
@roseeichelmann roseeichelmann requested a review from johnclary July 8, 2024 17:09
Copy link
Member

@johnclary johnclary left a comment

Choose a reason for hiding this comment

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

😍

Copy link
Contributor

@mddilley mddilley 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 the cleanup!! 🧹 🙏 I found one error when starting this locally, but I'm not sure if I have the latest code in here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@roseeichelmann I'm getting this error when starting VZE. I merged the latest from data-model-v2, but maybe I should be pulling some newer code in from somewhere. Let me know, and I'll turn this into an approval.

Screenshot 2024-07-30 at 10 50 54 AM

Copy link
Member

Choose a reason for hiding this comment

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

oops—it does appear some of those widgets were load bearing 😮

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for catching that mike! i just brought back the two widgets we actually use

Copy link
Contributor

@mddilley mddilley left a comment

Choose a reason for hiding this comment

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

I retested, and every view loaded for me! Thanks again for carving out this code, Rose! 🙏 🚢 🎉

@mddilley
Copy link
Contributor

mddilley commented Aug 1, 2024

Almost certainly a product of the merge party 🥳, but I saw this one compile warning.

Screenshot 2024-08-01 at 10 34 54 AM

Copy link
Member

@johnclary johnclary left a comment

Choose a reason for hiding this comment

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

it's all working for me! thanks @mddilley for catching those busted widgets.

🚢
🚢
🚢
🚢
🚢

@johnclary
Copy link
Member

Almost certainly a product of the merge party 🥳, but I saw this one compile warning.

ah yes—i saw that this is already fixed in the soft deletes branch 👍 🙏

@roseeichelmann roseeichelmann merged commit cd66128 into data-model-v2 Aug 1, 2024
@roseeichelmann roseeichelmann deleted the 17392_remove_unused_components branch August 1, 2024 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tech debt] Remove unused chart components from VZE
4 participants