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

[Dashboard Removal] Migrated Dashboard #14741

Merged
merged 14 commits into from
Apr 4, 2019

Conversation

jesusbotella
Copy link
Contributor

@jesusbotella jesusbotella commented Mar 13, 2019

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 is dashboard.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:

dashboard: [
rootDir('lib/assets/javascripts/dashboard/dashboard.js'),
...glob.sync(rootDir('assets/stylesheets/dashboard/*.scss')),
rootDir('assets/stylesheets/editor-3/_scroll-view.scss'),
rootDir('node_modules/internal-carto.js/themes/scss/entry.scss')
],

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

@jesusbotella jesusbotella force-pushed the remove-newdashboardfeature-ff branch from 5a65189 to c2caee7 Compare April 1, 2019 14:57
@jesusbotella jesusbotella changed the title Old Migrated Dashboard removal [Dashboard Removal] Migrated Dashboard Apr 4, 2019
@jesusbotella jesusbotella marked this pull request as ready for review April 4, 2019 08:24
@jesusbotella
Copy link
Contributor Author

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

Copy link
Contributor

@javitonino javitonino left a 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' %>
Copy link
Contributor

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)

Copy link
Contributor Author

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:

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

@jesusbotella jesusbotella merged commit e939bee into master Apr 4, 2019
@jesusbotella jesusbotella deleted the remove-newdashboardfeature-ff branch April 4, 2019 15:06
@alonsogarciapablo
Copy link
Contributor

🔥🔥🔥🔥?

@jesusbotella
Copy link
Contributor Author

It is the first stage, but yes, the migrated dashboard is no longer there :)

@alonsogarciapablo
Copy link
Contributor

👏 👏 👏

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.

3 participants