-
Notifications
You must be signed in to change notification settings - Fork 651
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
[Dashboard Removal] Migrated Dashboard #14741
Conversation
a1a39b3
to
a0643a0
Compare
5a65189
to
c2caee7
Compare
This PR is ready to review 🎉 It'd be great if @javitonino can give it a look just in case I missed something to remove and review the FF removal. And for the FrontEnd part, I invoke @rjimenezda as we worked on the dashboard migration to give it a look. cc/ @VictorVelarde |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found a 🐛
<% if @has_engine_enabled %> | ||
<%= javascript_include_tag 'api_keys_new' %> | ||
<%= javascript_include_tag 'common', 'common_vendor', 'api_keys_new' %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is incorrect. I think common
and common-vendor
should be always included (they were previously included if you had the new dashboard). I feel this will break the assets for people without engine (e.g: FREEs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refactored a bit this page when including the new header and footer, and the thing is that no assets are needed for people without Engine enabled!
When there's no engine enabled, it includes api_keys_cta
partial template here:
cartodb/app/views/admin/client_applications/api_key.html.erb
Lines 41 to 45 in b67c27f
<% if @has_engine_enabled %> | |
<div class="api-keys js-api-keys-new"></div> | |
<% else %> | |
<%= render :partial => 'api_keys_cta' %> | |
<% end %> |
Which according to the template, it doesn't need any JavaScript to render the partial or any of the content there.
🔥🔥🔥🔥? |
It is the first stage, but yes, the migrated dashboard is no longer there :) |
👏 👏 👏 |
This PR removes the migrated dashboard page from the repository.
This is not the old dashboard contained within
cartodb
. That one will be removed in another PR.This migrated dashboard page is within
lib/assets/javascripts/dashboard
and its entry point isdashboard.js
.This dashboard page is not used anymore because of the new dashboard has been put in place. So, it's time to clean it.
The dashboard bundle was compiled with Webpack and its dependencies were defined in:
cartodb/webpack/v4/entryPoints.js
Lines 22 to 27 in 9659826
So, doing an static analysis, we got the dependency files to remove that were detailed in https://github.com/CartoDB/product/issues/289 and removed in this pull request.
The removal is safe because we discarded the files being used in dashboard and other entry points so that we don't remove them causing a failure when building the bundle.
https://github.com/CartoDB/product/issues/289