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

Refactors JS utilities; separates IE concerns #583

Closed
wants to merge 1 commit into from

Conversation

anselmbradford
Copy link
Member

  • Removes unused util/util methods.
  • Moves cookie code to its own CRUD module.
  • Fixes Rails cookie writing settings - domains/subdomains, URL-encoding #565 - Removes server-side handling of Google Translate cookie
    and consolidates cookie handling in google-translate-manager.
  • Moves isTranslated method to google-translate-manager.
  • Updates “close” button method to “clear” to go with new naming.
  • Pads header popup title text so (x) button doesn’t risk overlapping.
  • Gives alert close button min height/width so that size doesn’t change
    as (x) icon is loading.
  • Implements addEventListener on alert so events can be attached to it
    being dismissed.
  • Fixes Provide upgrade message for IE8 users #573 - Creates IE8-specific override stylesheet and pulls :before and :after
    selectors out of main stylesheet into this one. Also provides overrides
    to elements that have box-shadows but no border by providing a border
    instead (alert and popup).
  • Updates modernizr custom build to include html5-shiv and include all core features used on the site.
  • Fixes Can we remove the gsub in requirejs_include_tag in the footer? #551 - Removes gsub from requirejs include tag.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1a9caab on js-utility-updates into abba69d on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 2164790 on js-utility-updates into abba69d on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d6e361f on js-utility-updates into abba69d on master.

@anselmbradford
Copy link
Member Author

Browser alert looks like:
screen shot 2014-09-08 at 5 34 36 pm

Links to http://browser-update.org/update.html

- Removes unused util/util methods.
- Moves cookie code to its own CRUD module.
- Fixes #565 - Removes server-side handling of Google Translate cookie
and consolidates cookie handling in google-translate-manager.
- Moves isTranslated method to google-translate-manager.
- Updates “close” button method to “clear” to go with new naming.
- Pads header popup title text so (x) button doesn’t risk overlapping.
- Gives alert close button min height/width so that size doesn’t change
as (x) icon is loading.
- Implements addEventListener on alert so events can be attached to it
being dismissed.
- Fixes #573 - Creates IE8-specific override stylesheet and pulls :before and :after
selectors out of main stylesheet into this one. Also provides overrides
to elements that have box-shadows but no border by providing a border
instead (alert and popup).
- Updates modernizr custom build to include html5-shiv and include all core features used on the site.
- Removes gsub from requirejs include tag.
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1f0a83b on js-utility-updates into abba69d on master.


// Create a cookie when the alert is closed that will hide the alert for the next day.
function _alertClosed() {
cookie.create('browser-upgrade-notice', 'true', true, 1);
Copy link
Member Author

Choose a reason for hiding this comment

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

Behavior for upgrade notice is when alert is closed a cookie is set for a expiration of one day in the future, so it won't show till the next day. It will continually show till the close (x) is clicked on the alert, however.

@anselmbradford
Copy link
Member Author

@monfresh Ready for review and merge. Thanks! Mostly frontend, but touches some backend a little. This should be rolled over to SMC-Connect too as I have access to an actual machine running IE and this contains important IE updates (sooo much easier with a machine than a VM).

@monfresh
Copy link
Member

monfresh commented Sep 8, 2014

Could you please break this down into smaller chunks? There's too much going on here that makes it hard to review. Ideally, pull requests should focus on one topic only. General JS code cleanup should be one PR, translation-related stuff should be another PR, and so on. The alert for IE8 users is a new feature and should definitely be in its own PR since it needs some thought, and shouldn't hold up a PR that's mainly just JS code cleanup. Makes sense?

@anselmbradford anselmbradford deleted the js-utility-updates branch September 8, 2014 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants