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.

Sign up for GitHub

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

Jump to bottom

fix: Alert can't be dismissed #2216

Merged
merged 7 commits into from
Aug 28, 2022
Merged

fix: Alert can't be dismissed #2216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e63e08d
fix: Dismissing alert not working
flacial Aug 24, 2022
d957a9a
test: Update snapshots
flacial Aug 24, 2022
f241daf
test: Create Alert test
flacial Aug 24, 2022
e460034
test: Update test cases
flacial Aug 24, 2022
6f3217c
Merge branch 'master' into 2215-fix-cant-dismiss-an-alert
flacial Aug 24, 2022
874b3ec
Merge branch 'master' into 2215-fix-cant-dismiss-an-alert
flacial Aug 25, 2022
5f31a3b
Merge branch 'master' into 2215-fix-cant-dismiss-an-alert
flacial Aug 28, 2022
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
50 changes: 18 additions & 32 deletions __tests__/__snapshots__/storyshots.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -908,7 +908,7 @@ exports[`Storyshots Components/AdminLessonSideNav Basic 1`] = `

exports[`Storyshots Components/Alert Info 1`] = `
<div
className="alert d-flex justify-content-between mt-3 alert alert-primary"
className="fade alert alert-primary show"
role="alert"
>
<div
Expand All @@ -931,9 +931,15 @@ exports[`Storyshots Components/Alert Info 1`] = `

exports[`Storyshots Components/Alert Info With Link And Dismiss 1`] = `
<div
className="alert d-flex justify-content-between mt-3 alert alert-primary"
className="fade alert alert-primary alert-dismissible show"
role="alert"
>
<button
aria-label="Close alert"
className="btn-close"
onClick={[Function]}
type="button"
/>
<div
className="d-flex align-items-center gap-3"
>
Expand All @@ -957,25 +963,12 @@ exports[`Storyshots Components/Alert Info With Link And Dismiss 1`] = `
View Instructions
</a>
</div>
<button
className="info_image alert-dismiss"
data-testid="dismiss-info"
onClick={[Function]}
role="dismiss"
>
<img
className="img-fluid"
height={24}
src="/assets/curriculum/icons/dismiss-info.svg"
width={24}
/>
</button>
</div>
`;

exports[`Storyshots Components/Alert Urgent 1`] = `
<div
className="alert d-flex justify-content-between mt-3 alert alert-danger"
className="fade alert alert-danger show"
role="alert"
>
<div
Expand All @@ -998,9 +991,15 @@ exports[`Storyshots Components/Alert Urgent 1`] = `

exports[`Storyshots Components/Alert Urgent With Icon And Dismiss 1`] = `
<div
className="alert d-flex justify-content-between mt-3 alert alert-danger"
className="fade alert alert-danger alert-dismissible show"
role="alert"
>
<button
aria-label="Close alert"
className="btn-close"
onClick={[Function]}
type="button"
/>
<div
className="d-flex align-items-center gap-3"
>
Expand All @@ -1024,19 +1023,6 @@ exports[`Storyshots Components/Alert Urgent With Icon And Dismiss 1`] = `
View NPM Package
</a>
</div>
<button
className=" alert-dismiss"
data-testid="dismiss-urgent"
onClick={[Function]}
role="dismiss"
>
<img
className="img-fluid"
height={24}
src="/assets/curriculum/icons/dismiss-urgent.svg"
width={24}
/>
</button>
</div>
`;

Expand Down Expand Up @@ -28050,7 +28036,7 @@ exports[`Storyshots Pages/Login Login Error 1`] = `
className="form-group"
>
<div
className="alert d-flex justify-content-between mt-3 alert alert-danger"
className="fade alert alert-danger show"
role="alert"
>
<div
Expand Down Expand Up @@ -28527,7 +28513,7 @@ exports[`Storyshots Pages/Signup Signup Errors 1`] = `
className="card-text"
/>
<div
className="alert d-flex justify-content-between mt-3 alert alert-danger"
className="fade alert alert-danger show"
role="alert"
>
<div
Expand Down
2 changes: 1 addition & 1 deletion __tests__/pages/__snapshots__/login.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ exports[`Login Page Should set alert visible on invalid credentials 1`] = `
class="form-group"
>
<div
class="alert d-flex justify-content-between mt-3 alert alert-danger"
class="fade alert alert-danger show"
role="alert"
>
<div
Expand Down
2 changes: 1 addition & 1 deletion __tests__/pages/__snapshots__/signup.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ exports[`Signup Page Should render errors on fail 1`] = `
class="card-text"
/>
<div
class="alert d-flex justify-content-between mt-3 alert alert-danger"
class="fade alert alert-danger show"
role="alert"
>
<div
Expand Down
72 changes: 24 additions & 48 deletions __tests__/pages/curriculum/__snapshots__/[lessonSlug].test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,14 @@ exports[`Lesson Page Should correctly render challenges page for students who ha
class="alerts-container p-0"
>
<div
class="alert d-flex justify-content-between mt-3 alert alert-primary"
class="fade alert alert-primary alert-dismissible show"
role="alert"
>
<button
aria-label="Close alert"
class="btn-close"
type="button"
/>
<div
class="d-flex align-items-center gap-3"
>
Expand All @@ -246,22 +251,16 @@ exports[`Lesson Page Should correctly render challenges page for students who ha
View Instructions
</a>
</div>
<button
class="info_image alert-dismiss"
data-testid="dismiss-info"
role="dismiss"
>
<img
height="24"
src="/assets/curriculum/icons/dismiss-info.svg"
width="24"
/>
</button>
</div>
<div
class="alert d-flex justify-content-between mt-3 alert alert-danger"
class="fade alert alert-danger alert-dismissible show"
role="alert"
>
<button
aria-label="Close alert"
class="btn-close"
type="button"
/>
<div
class="d-flex align-items-center gap-3"
>
Expand All @@ -277,17 +276,6 @@ exports[`Lesson Page Should correctly render challenges page for students who ha
Please upgrade your CLI client by running npm update c0d3.
</div>
<button
class=" alert-dismiss"
data-testid="dismiss-urgent"
role="dismiss"
>
<img
height="24"
src="/assets/curriculum/icons/dismiss-urgent.svg"
width="24"
/>
</button>
</div>
</div>
<div
Expand Down Expand Up @@ -933,9 +921,14 @@ exports[`Lesson Page Should render correctly with valid lesson route 1`] = `
class="alerts-container p-0"
>
<div
class="alert d-flex justify-content-between mt-3 alert alert-primary"
class="fade alert alert-primary alert-dismissible show"
role="alert"
>
<button
aria-label="Close alert"
class="btn-close"
type="button"
/>
<div
class="d-flex align-items-center gap-3"
>
Expand All @@ -958,22 +951,16 @@ exports[`Lesson Page Should render correctly with valid lesson route 1`] = `
View Instructions
</a>
</div>
<button
class="info_image alert-dismiss"
data-testid="dismiss-info"
role="dismiss"
>
<img
height="24"
src="/assets/curriculum/icons/dismiss-info.svg"
width="24"
/>
</button>
</div>
<div
class="alert d-flex justify-content-between mt-3 alert alert-danger"
class="fade alert alert-danger alert-dismissible show"
role="alert"
>
<button
aria-label="Close alert"
class="btn-close"
type="button"
/>
<div
class="d-flex align-items-center gap-3"
>
Expand All @@ -989,17 +976,6 @@ exports[`Lesson Page Should render correctly with valid lesson route 1`] = `
Please upgrade your CLI client by running npm update c0d3.
</div>
<button
class=" alert-dismiss"
data-testid="dismiss-urgent"
role="dismiss"
>
<img
height="24"
src="/assets/curriculum/icons/dismiss-urgent.svg"
width="24"
/>
</button>
</div>
</div>
<div
Expand Down
28 changes: 28 additions & 0 deletions components/Alert/Alert.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import React from 'react'
import { render, screen } from '@testing-library/react'
import Alert from '.'
import userEvent from '@testing-library/user-event'

// Imported to be able to use .toBeInTheDocument()
import '@testing-library/jest-dom'

describe('Alert component', () => {
it('should hide the alert on dismiss', async () => {
render(
<Alert
alert={{
id: 0,
text: 'Set up your computer to submit challenges.',
type: 'info',
url: 'https://www.notion.so/JS-0-Foundations-a43ca620e54945b2b620bcda5f3cf672#b45ed85a95e24c9d9fb784afb7a46bcc',
urlCaption: 'View Instructions'
}}
onDismiss={() => {}}
/>
)

await userEvent.click(screen.getByRole('button'))

expect(screen.queryByRole('alert')).not.toBeInTheDocument()
})
})
37 changes: 15 additions & 22 deletions components/Alert/Alert.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import React from 'react'
import React, { useState } from 'react'
import NavLink from '../NavLink'
import { Alert as AlertType } from '../../graphql/'
import _ from 'lodash'
import styles from './alerts.module.scss'
import Image from 'next/image'
import { Alert as AlertBS } from 'react-bootstrap'

type Props = {
alert: AlertType
Expand All @@ -21,16 +22,20 @@ const alertIconMap: { [type: string]: string } = {
}

const Alert: React.FC<Props> = ({ alert, onDismiss }) => {
const [show, setShow] = useState(true)

const { text, type, url, urlCaption, id } = alert
const icon = alertIconMap[_.defaultTo(type, '-1')]
const alertClasses =
type === 'urgent' ? 'alert alert-danger' : `alert alert-primary`
const imageClass = type === 'urgent' ? '' : styles.info_image

return (
<div
className={`${styles.alert} d-flex justify-content-between mt-3 ${alertClasses}`}
role="alert"
return show ? (
<AlertBS
variant={`${type === 'urgent' ? 'danger' : 'primary'}`}
onClose={() => {
setShow(false)
onDismiss && onDismiss(id)
Copy link
Collaborator

@HS-90 HS-90 Aug 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just out of curiosity and to confirm, does onDismiss handle whether or not the alert will be displayed for particular user in the future(if the user clicks the x button to dismiss)?

It appears the frontend is handled by setShow

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it doesn't handle it. It gets called when the alert is closed. It's useful when we want to perform a certain action when the alert closes.

}}
dismissible={!!onDismiss}
>
<div className="d-flex align-items-center gap-3">
<div className={imageClass}>
Expand All @@ -43,21 +48,9 @@ const Alert: React.FC<Props> = ({ alert, onDismiss }) => {
</NavLink>
)}
</div>
{onDismiss && (
<button
role="dismiss"
className={`${imageClass} ${styles['alert-dismiss']}`}
data-testid={`dismiss-${type}`}
onClick={() => onDismiss(id)}
>
<Image
src={`/assets/curriculum/icons/dismiss-${type}.svg`}
width={24}
height={24}
/>
</button>
)}
</div>
</AlertBS>
) : (
<></>
)
}

Expand Down
8 changes: 4 additions & 4 deletions components/AlertsDisplay/AlertsDisplay.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('Alerts Display Component', () => {
<AlertsDisplay alerts={alerts} page="curriculum" />
)

const displayedAlerts = getAllByRole('alert')
const displayedAlerts = getAllByRole('button')
expect(displayedAlerts.length).toEqual(1)
})

Expand All @@ -48,13 +48,13 @@ describe('Alerts Display Component', () => {
const { getAllByRole } = render(
<AlertsDisplay alerts={alerts} page="curriculum" />
)
let displayedAlerts = getAllByRole('alert')
let displayedAlerts = getAllByRole('button')
expect(displayedAlerts.length).toEqual(2)
const firstDismiss = getAllByRole('dismiss')[0]
const firstDismiss = getAllByRole('button')[0]
act(() => {
fireEvent.click(firstDismiss)
})
displayedAlerts = getAllByRole('alert')
displayedAlerts = getAllByRole('button')
expect(displayedAlerts.length).toEqual(1)
})
})
Loading