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

Remove ActiveAdmin #196

Merged
merged 3 commits into from
Sep 27, 2023
Merged

Remove ActiveAdmin #196

merged 3 commits into from
Sep 27, 2023

Conversation

marlena-b
Copy link
Collaborator

Describe your change in plain English.

Removes ActiveAdmin from the project.

Link to the issue

Fixes #13

kasugaijin
kasugaijin previously approved these changes Sep 26, 2023
View reviewed changes
Copy link
Collaborator

@kasugaijin kasugaijin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me! See below for one suggestion before merging.

kasugaijin
kasugaijin reviewed Sep 26, 2023
View reviewed changes
Copy link
Collaborator

@kasugaijin kasugaijin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think we can remove these also
pin "chartkick", to: "chartkick.js"
pin "Chart.bundle", to: "Chart.bundle.js"
in config/importmap.rb

these were used to make some graphs in active admin dashboard

@kasugaijin kasugaijin dismissed their stale review September 26, 2023 22:19

one more change required

@marlena-b
Copy link
Collaborator Author

Thank you @kasugaijin! I missed that, it's fixed now

@marlena-b marlena-b marked this pull request as draft September 27, 2023 09:18
@marlena-b marlena-b marked this pull request as ready for review September 27, 2023 10:37
@marlena-b
Copy link
Collaborator Author

Tests are failing but looks like they are unrelated to my changes (broken on main)

@edwinthinks
Copy link
Collaborator

Yup @marlena-b it was just fixed in main, I feel pretty good about merging this in

@edwinthinks edwinthinks merged commit 2c67bfe into main Sep 27, 2023
@edwinthinks edwinthinks deleted the remove-active-admin branch September 27, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edwinthinks edwinthinks Awaiting requested review from edwinthinks

@Thrillberg Thrillberg Awaiting requested review from Thrillberg

@kasugaijin kasugaijin Awaiting requested review from kasugaijin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove active admin
3 participants
@marlena-b @edwinthinks @kasugaijin