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 dependency on $.browser #7

Merged
merged 8 commits into from Jul 23, 2015
Merged

Remove dependency on $.browser #7

merged 8 commits into from Jul 23, 2015

Conversation

@fofr
Copy link

@fofr fofr commented Jul 21, 2015

Extract the $.browser logic and tests into a shim and replace $.browser calls with window.NOMENSA.browser ones.

The browser based tests are still needed to identify Firefox versions with a known flash bug that interferes with SWFObject. This can't easily be handled using feature detection.

There are also MSIE tests which add classes for styling, the default styles use these to trigger hasLayout.

Includes whitespace changes, recommend reviewing using ?w=1

EDIT: Tested on Static and https://wwww.gov.uk/government/news/ramadan-2015-david-camerons-message

fofr added 6 commits Jul 20, 2015
* Extract $.browser logic from jQuery
* Based on https://gist.github.com/adeelejaz/4714079
* Include only the browser detection needed by the player
* Add browser to NOMENSA namespace rather than jQuery
* Completely detach from jQuery
Use the local browser detection rather than the deprecated jQuery one.
Omitting early regexp tests would create false positives.

* Put back opera and chrome regexps
* Use user agents from
https://github.com/jquery/jquery/blob/1.7.2/test/data/ua.txt
* Expose the UA test for testing (it was previously exposed in jQuery)
@fofr fofr force-pushed the remove-browser branch from 6c6f956 to a9f457a Jul 21, 2015
fofr added 2 commits Jul 21, 2015
* Remove dependency on jQuery $.browser, replace with NOMENSA.browser
* Add changelog
Re-run `./compress_js.sh`
@fofr fofr force-pushed the remove-browser branch from a9f457a to 3e1cc9c Jul 21, 2015
@tombye

This comment has been minimized.

Copy link

@tombye tombye commented on core/javascript/jquery.browser.js in 7a5dc11 Jul 22, 2015

Is jQuery migrator used here? (I can only see the gist).

If not, might be worth removing this reference to make that clear.

This comment has been minimized.

Copy link
Author

@fofr fofr replied Jul 22, 2015

The gist is derived from jQuery migrator – rather than being custom written for this purpose.

@tombye
Copy link

@tombye tombye commented Jul 22, 2015

This is really nice reading - very clear what the changes are.

Apart from my one question (as a comment) LGMT 👍

@dsingleton
Copy link

@dsingleton dsingleton commented Jul 23, 2015

The changes look good to me, especially the testing 👍

I checked this out locally to run the tests (with ./run_tests.sh and i'm seeing (unrelated) failures on this branch and master.

jquery.player tests (integration) : should not call the YoutubePlayer constructor when using Vimeo TypeError: 'null' is not an object (evaluating 'media.container')

I think I may be missing a setup step, but i'm assuming they pass locally for you?

It would be good to have CI for this repo, not a blocker for this PR if the tests do pass, but worth adding for general confidence, given this a repo we touch irregularly.

(Top commits btw, this was a pleasure to review)

@fofr
Copy link
Author

@fofr fofr commented Jul 23, 2015

@dsingleton The unrelated test failure is also failing on master, and is probably related to a change in an external Vimeo API since the tests were written – which I discussed with @tombye. I've left out fixing that test from this PR because it's unrelated, but it should be put right.

This is a repo we've previously pushed upstream (and I'd like to push these changes too), which is something to consider when setting up CI.

@dsingleton
Copy link

@dsingleton dsingleton commented Jul 23, 2015

In which case i'm happy to merge this, and if we can add CI, fix the test and push upstream, then those are all bonuses.

dsingleton added a commit that referenced this pull request Jul 23, 2015
Remove dependency on $.browser
@dsingleton dsingleton merged commit 4cadee7 into master Jul 23, 2015
@dsingleton dsingleton deleted the remove-browser branch Jul 23, 2015
fofr added a commit to alphagov/govuk_frontend_toolkit that referenced this pull request Jul 23, 2015
Removes dependency on `$.browser`
alphagov/Accessible-Media-Player#7
fofr added a commit to alphagov/govuk_frontend_toolkit that referenced this pull request Jul 23, 2015
- Update Accessible Media Player to remove dependency on $.browser (PR
#206)

alphagov/Accessible-Media-Player#7
fofr added a commit to alphagov/static that referenced this pull request Jul 24, 2015
jQuery.history is used only by jQuery.tabs. It's use of
the deprecated jQuery.browser is blocking the upgrade of
jQuery. There is a plan to design away tabs.

Replace uses with a `historyBrowserShim`. The same approach as
alphagov/Accessible-Media-Player#7

* Refer to local shim rather than jQuery
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.