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

[DT-934] Create a banner similar to the Terra Banner on DUOS to inform users that chrome is our preferred browser #2710

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ ENV PATH /usr/src/app/node_modules/.bin:$PATH

# install and cache app dependencies
COPY src /usr/src/app/src
COPY types /usr/src/app/types
COPY public /usr/src/app/public
COPY package.json /usr/src/app/package.json
COPY package-lock.json /usr/src/app/package-lock.json
Expand Down
11 changes: 11 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"ajv": "8.17.1",
"ajv-formats": "3.0.1",
"axios": "1.7.7",
"browser-version-detection": "^2.1.8",
Copy link
Contributor

@fboulnois fboulnois Nov 14, 2024

Choose a reason for hiding this comment

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

Suggested change
"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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused here ... the latest release version I see is 3.1.0 that is from last year. Is there a reason we're looking at 2.1.8 (which I don't even see in the list of releases)? I do see that NPM sees 2.1.8, which is extra confusing.

Copy link
Contributor

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.

"dompurify": "3.1.7",
"js-file-download": "0.4.12",
"lodash": "4.17.21",
Expand Down
35 changes: 35 additions & 0 deletions src/components/ActionableAlert.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import React from 'react';
import './Alert.css';
import CloseIcon from '@mui/icons-material/Close';
import { Button } from '@mui/material';

export interface ActionableAlertProps {
id: string;
type: string;
title: string;
description: string;
onClose?: () => void;
actionText: string;
onClick?: () => void;
}

export const ActionableAlert = (props: ActionableAlertProps) => {
const {id, type, title, description, onClose, actionText, onClick} = props;
return (
<div id={`${id}_alert`} className={`alert-wrapper ${type}`} style={{border: '1px solid red', borderRadius: '5px'}}>
{onClose && <span
style={{float: 'right', fontWeight: 'bolder', fontSize: 24, cursor: 'pointer'}}
onClick={onClose} ><CloseIcon/></span> }
{title && <h4 id={`${id}_title`} className="alert-title">{title}</h4>}
{description && <span id={`${id}_description`} className="alert-description">{description}</span>}
{actionText && <Button
variant='outlined'
color='error'
onClick={onClick}
sx={{fontSize: 10, marginTop: 2}}
>
{actionText}
</Button>}
</div>
);
};
39 changes: 28 additions & 11 deletions src/components/SignInButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {History} from 'history';
import {OidcUser} from '../libs/auth/oidcBroker';
import {DuosUser} from '../types/model';
import {DuosUserResponse} from '../types/responseTypes';
import {ActionableAlert} from '../components/ActionableAlert';
import {detectBrowser} from 'browser-version-detection/src/browser-detection';

interface SignInButtonProps {
history: History;
Expand All @@ -39,6 +41,13 @@ export const SignInButton = (props: SignInButtonProps) => {
const {history} = props;
const [isLoading, setIsLoading] = useState<boolean>(false);

const browser = detectBrowser(window.navigator);
const browserName = browser.name;
const checkBrowser = () => {
return (!isEmpty(browserName) && browserName !== 'Chrome' && browserName !== 'Firefox');
};
const [showUnsupported, setShowUnsupported] = useState<boolean>(checkBrowser());

// Utility function called in the normal success case and in the undocumented 409 case
// Check for ToS Acceptance - redirect user if not set.
const checkToSAndRedirect = async (redirectPath: string | null) => {
Expand Down Expand Up @@ -205,19 +214,27 @@ export const SignInButton = (props: SignInButtonProps) => {

return (
<div>
{isEmpty(errorDisplay)
? <div>
{showUnsupported ?
<ActionableAlert
id="dialog"
type="danger"
title={'DUOS may not function correctly in this browser.'}
description={'If you are experiencing issues, please try using Google Chrome.'}
onClose={() => setShowUnsupported(false)}
actionText={'Download Chrome now'}
onClick={() => window.open('https://www.google.com/chrome/')}/> :
isEmpty(errorDisplay) ? <div>
{signInElement()}
</div>
: <div className="dialog-alert">
<Alert
id="dialog"
type="danger"
title={(errorDisplay as ErrorInfo).title || 'Error'}
description={(errorDisplay as ErrorInfo).description || ''}
onClose={() => setErrorDisplay({})}
/>
</div>
: <div className="dialog-alert">
<Alert
id="dialog"
type="danger"
title={(errorDisplay as ErrorInfo).title || 'Error'}
description={(errorDisplay as ErrorInfo).description || ''}
onClose={() => setErrorDisplay({})}
/>
</div>
}
</div>
);
Expand Down
3 changes: 2 additions & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
}
},
"include": [
"src"
"src",
"types/index.d.ts"
],
"plugins": ["@typescript-eslint", "import"]
}
3 changes: 3 additions & 0 deletions types/index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
declare module "browser-version-detection/src/browser-detection" {
export function detectBrowser(nav: { userAgent: string }): any
}
Loading