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

Check browser version, blacklisting outdated IE #8385

Merged
merged 1 commit into from Dec 29, 2014

Conversation

vicvega
Copy link
Contributor

@vicvega vicvega commented Nov 24, 2014

Add an alert message in case of outdated browser.
Quite the same as #7597, but implementing a blacklist for outdated IE (version < 10)
screenshot from 2014-11-24 17 10 19

@TeatroIO
Copy link

I've prepared a stage. Click to open.

@bbodenmiller
Copy link
Contributor

Replaces #8348.

padding: 10px;
text-align: center;
background: #C67;
// background: #F51;
Copy link
Member

Choose a reason for hiding this comment

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

remove commented code

@vicvega
Copy link
Contributor Author

vicvega commented Dec 2, 2014

I also rebased on current master to make it mergeable

@randx let me know if I can do something else

@jvanbaarsen
Copy link
Contributor

@vicvega Can you squash the commits to one commit?

@vicvega
Copy link
Contributor Author

vicvega commented Dec 15, 2014

@jvanbaarsen I've squashed the commits and rebased on current master

@jvanbaarsen
Copy link
Contributor

@vicvega Thanks!

@dblessing
Copy link
Member

Linking to the original issue where Dmitriy and myself commented. #7597

I think this PR gets pretty close to what we talked about. The only thing is Dmitriy suggested IE 6-7 and lower and this is for 10 and lower.

I'm torn on these type of warnings. There's nothing more annoying than a 'browser may be incompatible' message IMHO. Should we make this warning dismiss-able? How broken is IE 10 and lower with current GitLab?

@jvanbaarsen
Copy link
Contributor

@dblessing I know that IE 8 and IE9 also giving trouble from time to time, I think we should give this warning to at least ie 6/7 and 8 users. And preferably IE9.
--edit:
GitLab officially only supports IE10+: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/install/requirements.md#supported-web-browsers So in order to prevent issues flowing in from IE9 users, im all for this PR! 👍

@vicvega
Copy link
Contributor Author

vicvega commented Dec 15, 2014

@dblessing with IE8 (unfortunately still present in my company) gitlab is totally unusable

Here is how it looks like
screenshot from 2014-12-15 14 51 18

It is all messed up, with several javascript errors...

and here is the same page (not joking, it is really the same page!) in firefox 34
screenshot from 2014-12-15 14 51 37

@bbodenmiller
Copy link
Contributor

How about keeping the message for all versions through IE 10 but making it dismissible like the SSH keys message?

@jvanbaarsen
Copy link
Contributor

@bbodenmiller Well, I really think this is an important notice. So being able to dismiss the notice will have the effect that people dismiss the notice, and after a while forget that they use a crappy browser, and start reporting bugs that are no bugs but just the result of using a crappy browser.

What do you guys think about trying out the permanent notice, if we get comments about it being really annoying, we can change it in a future release.

@bbodenmiller
Copy link
Contributor

👍

@vicvega
Copy link
Contributor Author

vicvega commented Dec 16, 2014

start reporting bugs that are no bugs but just the result of using a crappy browser.

It's exactly what happened in my company 😄

I totally agree with @jvanbaarsen

@jvanbaarsen
Copy link
Contributor

@vicvega Can you please make sure its mergeable (sorry :( )?

@vicvega
Copy link
Contributor Author

vicvega commented Dec 16, 2014

@jvanbaarsen done (it was just the changelog)
I rebased on current master

@vicvega
Copy link
Contributor Author

vicvega commented Dec 16, 2014

hmmm tests are failing?!!?
I just updated CHANGELOG and rebased on current master

@jvanbaarsen, is there something nasty on current master?

@jvanbaarsen
Copy link
Contributor

@vicvega Nah sometimes Semaphore is having hick-ups, i restarted the build for you. Im also looking into this issue since i see a rise in failing builds last day.

@vicvega
Copy link
Contributor Author

vicvega commented Dec 16, 2014

@jvanbaarsen ok good
all green now

@jvanbaarsen jvanbaarsen added this to the 7.7 milestone Dec 16, 2014
@vicvega
Copy link
Contributor Author

vicvega commented Dec 24, 2014

just rebased on current master

hmmm
I see a failing test on semaphore, but they all pass locally

@vicvega
Copy link
Contributor Author

vicvega commented Dec 24, 2014

I forced another push and all test are passing now
semaphore hiccup again?

anyway merry xmas!
🎄 🎅

@dzaporozhets dzaporozhets merged commit 84b40a3 into gitlabhq:master Dec 29, 2014
dzaporozhets added a commit that referenced this pull request Dec 29, 2014
Check browser version

Fix conflicts and merge #8385

See merge request !1357
@vicvega vicvega deleted the check_browser_version branch January 27, 2015 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants