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
Conversation
1a9caab
to
2164790
Compare
2164790
to
d6e361f
Compare
- 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.
d6e361f
to
1f0a83b
Compare
|
||
// 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); |
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.
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.
@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). |
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? |
and consolidates cookie handling in google-translate-manager.
as (x) icon is loading.
being dismissed.
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).