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 old, unused modals code #5068

Merged
merged 110 commits into from
Aug 24, 2015
Merged

Remove old, unused modals code #5068

merged 110 commits into from
Aug 24, 2015

Conversation

viddo
Copy link
Contributor

@viddo viddo commented Aug 17, 2015

Fixes #4673

This 🔥 🔥 🔥 code related to the now deprecated old modals.

Also removes a lot of implicit CSS, images, templates, models, views etc. that are no longer used.

Also saw some things that possibly could be removed too:

  • OauthController / front_layout , the only usage of old_common.css bundle, is this layout even used anymore? How can we verify that? If not, I'd suggest to 🔥 it and remove unncessary old_common styles.
  • fonts_ie.css.scss, really necessary anymore? Only used on public pages for some reason
  • is the ProximaNova still required?
  • considering old_common is getting phased out in favour of new styles, why not replace the submodule dir with a copy, so we can remove files easier as going forward? e.g. old_common/upload.css is no longer used
  • old_elements/views/dialog_base - references from inside cartodb.js makes it impossible to remove this form now, used for ui/common/share.js view, is it really required for anything outside of cartodb? If not I'd suggest to move related code inside cartodb boundary and keep cartodb.js a little bit cleaner

@xavijam can you review?
cc @CartoDB/frontend + regarding the questions listed ☝️

@viddo
Copy link
Contributor Author

viddo commented Aug 18, 2015

cc @iriberri we'll probably need to do a smoke test on this one. in theory, it's only code related to old modals, but considering the amount of removed code better to be on the safe side.

@@ -1,6 +1,7 @@
@import "../../variables/mixins";
@import "../../variables/colors";
@import "../../variables/sizes";
@import "../../map/map-sprite";
Copy link
Contributor

Choose a reason for hiding this comment

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

No more sprites please, let's try to leave and they are and if we need images, create them separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was migrated from app/assets/stylesheets/map/scratch_dialog.css.scss (now deleted) as it was. cc @javierarce

, let's try to leave and they are and if we need images

Not sure I understand the sentence entirely, do we leave it as it is for now or you want to change it? What would be the proper way of writing these rules?

Copy link
Contributor

Choose a reason for hiding this comment

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

Leave compass sprites as they are because we are still using it in the editor, but we shouldn't add more icons to them due to the fact that we will remove them when we have the new editor layout.

@xavijam
Copy link
Contributor

xavijam commented Aug 18, 2015

I've checked that we have two modernizr files in vendor/assets/javascripts/...
Could we use just one and remove the other?

@xavijam
Copy link
Contributor

xavijam commented Aug 18, 2015

Questions:

  1. Do we use respond.js? I've searched through the repository and I didn't find anything.
  2. Do we use spin.min.js? I feel, after this pull request, we are not using it anymore.

cc @CartoDB/frontend

@xavijam
Copy link
Contributor

xavijam commented Aug 18, 2015

Seems like we forgot to remove old geocoding model, in table.js:

...
// TODO: remove when new_modals is enabled for everybody
this.geocoder = new cdb.admin.Geocoding();
...

And of course, their bindings, for example:

// Geocoder binding
this.options.geocoder.bind('geocodingComplete geocodingError geocodingCanceled', function() {
   this.notice(_t('loaded'));
}, this);
this.add_related_model(this.options.geocoder);

There are some of this type.

@xavijam
Copy link
Contributor

xavijam commented Aug 18, 2015

In general terms, 🇺🇸, but it is difficult to evaluate the PR file by file. I encourage to pass all smoke tests related with editor view.

Is using the .min.js one so no point in having this file around
@viddo
Copy link
Contributor Author

viddo commented Aug 18, 2015

Geocoding is still used in table/header/options_menu.js, to check geocoder.isGeocoding(). I'll add it to the list of #5076 to consolidate those models too

@viddo
Copy link
Contributor Author

viddo commented Aug 18, 2015

  1. Do we use respond.js? I've searched through the repository and I didn't find anything.

No, doesn't look like it, removing.

  1. Do we use spin.min.js? I feel, after this pull request, we are not using it anymore.

It's still used in a handful of places (Search for new Spinner). Strangely enough, also used in cartodb.js: src/geo/ui/infowindow.js#L529, is it provided there or is some kind of implicit/hidden dependency for that view?

@iriberri
Copy link
Contributor

Ping me if you need my help.

Nicklas Gummesson added 3 commits August 20, 2015 11:46
Using font values from last time they were corrected, #4920.
- Had to leave the type values as-is since also used for local storage
keys.
- Added a new attr for the iconfont.
- Renamed is—new => is-new (single hyphen) to adhere to style guide.
- Removed iconFont-custom rules since that type is not used (anymore)
- Moved line height to the general rule since applies to all

Fixes #5126 cc @xavijam @MariaCheca
viddo added a commit that referenced this pull request Aug 24, 2015
@viddo viddo merged commit 1b7bff7 into master Aug 24, 2015
@viddo viddo deleted the 4673-rm-old-modals branch August 24, 2015 08:57
@viddo viddo restored the 4673-rm-old-modals branch August 24, 2015 08:57
@viddo viddo deleted the 4673-rm-old-modals branch August 24, 2015 08:57
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