-
Notifications
You must be signed in to change notification settings - Fork 134
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 browers list #634
check browers list #634
Conversation
} else { | ||
text = | ||
'Your browser version is outdated, recommend to use the latest Chrome or Edge to get better experience.' | ||
} |
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.
Do a simple i18n to avoid to use i18next library.
@@ -10,6 +10,8 @@ | |||
</head> | |||
<body> | |||
<noscript>You need to enable JavaScript to run this app.</noscript> | |||
<script src="%PUBLIC_URL%/supportedBrowsers.js"></script> | |||
<script src="%PUBLIC_URL%/checkBrowser.js"></script> |
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.
We can combine these 2 js together but that needs to use sed. As I know, the sed works differently between macOS and Linux.
ui/public/checkBrowser.js
Outdated
'您的浏览器版本已过期,推荐使用最新版本的 Chrome 或 Edge 浏览器以便获得更好的体验。' | ||
} else { | ||
text = | ||
'Your browser version is outdated, recommend to use the latest Chrome or Edge to get better experience.' |
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.
Sometimes the page is simply broken, prefer to avoid the soft "recommend"
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.
We also support Firefox
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.
Ok, how about "latest modern browsers"?
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.
Modern browser is a word for developers and users don't know what it means precisely. I prefer to explicitly list out our supported browsers
ui/public/checkBrowser.js
Outdated
function checkBrowser() { | ||
if (!window.__supported_browsers__.test(navigator.userAgent)) { | ||
var text | ||
if (navigator.language.indexOf('zh') === 0) { |
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.
IE6 ~ 10 even do not support NavigatorLanguage API, and I'm afraid it will be broken in such cases.
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.
Let me do some test.
ui/public/checkBrowser.js
Outdated
} | ||
|
||
const content = | ||
'<div style="background: yellow; width: 100%; position: absolute; top: 0; z-index: 4; padding: 8px; text-align: center; transition: top 2s;">' + |
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.
Transition needs later browser support and may not work in old browsers (please verify whether it will break things)
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.
So it just has no animation in the old browser, I guess, let me test it.
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.
Cool, if it doesn't break things then it would be great
ui/public/checkBrowser.js
Outdated
var text | ||
if (navigator.language.indexOf('zh') === 0) { | ||
text = | ||
'您的浏览器版本已过期,推荐使用最新版本的 Chrome 或 Edge 浏览器以便获得更好的体验。' |
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.
UTF8 characters might cause problems in some old browsers I guess, please test it against IE6
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.
Really, just know it, 😂 Where can I find an IE6 now... anyway, I will try it, maybe I can test it in IE11 first. (ant design only support IE11 for IEs)
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.
A note.. AFAIK TiDB Dashboard won't work on IE11 as well
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.
IE6 has a wrong charset detecting strategy https://support.microsoft.com/zh-cn/help/928847/internet-explorer-uses-the-wrong-character-set-when-it-renders-an-html (Honestly I have no ideas how to solve it for now)
ui/public/checkBrowser.js
Outdated
} | ||
|
||
const content = | ||
'<div style="background: yellow; width: 100%; position: absolute; top: 0; z-index: 4; padding: 8px; text-align: center; transition: top 2s;">' + |
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.
How about using red as the background to bring more attractions
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.
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.
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.
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.
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.
Cool!
Updated logic:
|
shell.task( | ||
'echo "window.__unsupported_browsers__ = $(browserslist-useragent-regexp "dead, IE 11" --allowHigherVersions)" > ./public/unSupportedBrowsers.js' | ||
) | ||
) |
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.
The generated full list is here: https://browserl.ist/?q=dead%2C+IE+11
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.
It won't work in some old Chrome versions as well. The list needs some tuning..
<div id="root"> | ||
<h1 id="title">Please update your browser</h1> | ||
<p id="desc"> | ||
Your browser isn't supported anymore. Update it to get the best TiDB |
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.
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.
The current implementation does not work well to meet the demand "For browsers that functionality will totally break, must not continue, asking user to change browser". It is still displaying soft recommendations. For example, we met a client that uses Chrome 5X. For such users, displaying a soft recommendation is not appropriate (and due to the soft color user might even not notified).
How about implementing a simple version first (I think you can open another PR)? The comprehensive implementation requires much more work to do compared to current PR, for example:
- Need to test the minimal supported browser version, for example, Chrome 10 definitely won't work. In such case, only displaying a recommendation is not enough.
- Need to check implementation against old browsers to see if the notification itself works. For example, IE6. Current implementation might even have problems in IE6 that cannot display the message correctly.
- Need to check implementation against our public path prefix feature for those images and the new
updateBrowser.html
web page. Current code doesn't seem to be compatible with it.. - Now this PR uses a simple redirection. It will cause misinformation after user switching to the required browser but copy-paste the URL again.
The simple version only needs to display an attractive & non-recommendation message when the current browser is not in our supported list. It's simply a wording change over your previous commits: recommend to use ... to get better experience
--> Some features may not work. Please use Chrome > xxx, Firefox > xxx, Edge > xxx or ....
.
/merge |
Sorry @AndreMouche, you don't have permission to trigger auto merge event on this branch. You are not a committer for the related sigs:Dashboard(slack). |
I am working on another PR. |
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
What did:
Principle: browserslist-useragent-regexp is a utility to compile browserslist query to a RegExp to test browser useragent.