Skip to content

Conversation

@vicvega
Copy link
Contributor

@vicvega vicvega commented Nov 18, 2014

Add an alert message in case of outdated browser.
Quite the same as #7597, but implementing a blacklist for ie6 and ie7

Here is a screenshot
screenshot from 2014-11-18 17 43 28

@TeatroIO
Copy link

I've prepared a stage. Click to open.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you not put this inside layouts/flash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes sure, I can move the message inside layout/flash

I put it in a separate file becasue it's not properly a flash message. Anyway, I reckon moving in layout/flash may be a cleaner solution... As you prefer

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option would be to rename flash to something more generic that characterizes both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good

Could be a layouts/messages containing something like

= render "layouts/flash"
= render "layouts/outdated_browser" if unsupported_browser?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me.

@cirosantilli
Copy link
Contributor

+1 for this idea (haven't reviewed implementation)

@vicvega
Copy link
Contributor Author

vicvega commented Nov 19, 2014

I've just moved the message under head panel.
It's cleaner and does not interfere with flash messages

Looks like this

screenshot from 2014-11-19 12 36 06

@vicvega
Copy link
Contributor Author

vicvega commented Nov 19, 2014

Modified css to have a message similar to ssh-no-key-message
Here it is

screenshot from 2014-11-19 15 46 31

I think it's very unlikely that the two messages (ssh-no-key-message and browser-alert) appear together, anyway here is a screenshot
screenshot from 2014-11-19 15 45 55

@cirosantilli
Copy link
Contributor

I kind of like the orange one.

@vicvega
Copy link
Contributor Author

vicvega commented Nov 19, 2014

Orange should be like this

screenshot from 2014-11-19 16 08 03

I'd prefer the red version (less shocking). Anyway no problem

Any other opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer:

GitLab may not work properly because you are using an outdated web browser. Please install a supported web browser for a better experience.

Specifically click here links in web development are frowned upon and Gitlab should be GitLab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bbodenmiller thanks for suggestion.
Done!

@bbodenmiller
Copy link
Contributor

Are your screenshots actually from IE6 or IE7. When I put IE11 in IE7 or IE8 document mode I get:

image

If I leave it in IE9+ document mode but set the user agent to IE6 or IE7 then I can see the warning. The real IE6 & IE7 however are going to see what it looks like when IE11 is in those document modes. Point is for IE6, IE7, & IE8 a full screen not support banner needs to be created that we are sure works in those web browsers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Somehow the link is linking whitespace:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for reporting
now it's fixed

@vicvega vicvega changed the title Check browser version, blacklisting ie6 end ie7 Check browser version, blacklisting outdated IE versions Nov 20, 2014
@vicvega
Copy link
Contributor Author

vicvega commented Nov 21, 2014

After @bbodenmiller suggestions here is an updated screenshot (from IE8)
screenshot from 2014-11-21 16 01 14

@bbodenmiller
Copy link
Contributor

Hmmm... not sure if it is a problem with @TeatroIO but I'm not seeing this on any pages now.

@vicvega
Copy link
Contributor Author

vicvega commented Nov 24, 2014

Hmm worksforme
Here is a screen with IE8 on teatroIO
screenshot from 2014-11-24 10 48 11

Choose a reason for hiding this comment

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

Indent the right brace the same as the start of the line where the left brace is.

@vicvega
Copy link
Contributor Author

vicvega commented Nov 24, 2014

I've messed up the history. I'm giong to open a new PR

@vicvega vicvega closed this Nov 24, 2014
@vicvega vicvega deleted the check_browser branch November 24, 2014 16:22
@bbodenmiller
Copy link
Contributor

Please add a link here when you add the new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

8 participants