-
Notifications
You must be signed in to change notification settings - Fork 4
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
[DT-934] Create a banner similar to the Terra Banner on DUOS to inform users that chrome is our preferred browser #2710
base: develop
Are you sure you want to change the base?
Conversation
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.
Looks good
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.
See below:
Updated visual: 11-13-2024.mov |
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.
See below:
@@ -14,6 +14,7 @@ | |||
"ajv": "8.17.1", | |||
"ajv-formats": "3.0.1", | |||
"axios": "1.7.7", | |||
"browser-version-detection": "^2.1.8", |
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.
"browser-version-detection": "^2.1.8", | |
"browser-version-detection": "2.1.8", |
We should lock this version down so it doesn't automatically change.
This version hasn't seen any updates for 7 years, and the implementation is straightforward, so I would continue to advocate not depending on a package to do this and instead using a simple regex for browser detection (which is ultimately what this library and the previous one do).
In general, browser detection is not recommended, although I know we also do this on Terra UI. It might be better to identify the specific features that Safari doesn't support, and long term work to either stop depending on these features or use polyfills for those features that we really need. This is a separate issue from showing the alert, so in the meantime we might still want to show the alert regardless of whether we are doing browser or feature detection.
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.
Aside from the library version question, I do agree in theory with Florian's feedback. To date, the only issue we've seen with Safari is variations in CSS rendering. I don't think we're using any browser-specific features, so to really tackle that question, we'd need a different kind of investigation effort.
Addresses
https://broadworkbench.atlassian.net/browse/DT-934
Summary
Displays a warning alert for users opening DUOS on a browser that's not Chrome or Firefox, user cannot click the Sign In button until Alert is closed
Screen.Recording.2024-11-05.at.9.49.38.AM.mov
Note: Updated the appearance of the alert to be less extreme - see comment below
Have you read Terra's Contributing Guide lately? If not, do that first.