-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat: refined ux update a taxonomy by downloading and uploading [FC-0036] #732
Changes from 84 commits
0c60c37
c273af1
047530d
303e7ba
3955e91
8176973
4f31714
df441fb
a0e92f0
dc8ed9e
120154e
318bbb7
a8c2fd5
70ac2a7
adacf23
5983953
7756c7f
4202c27
6bcd924
f318dfc
4c22a0a
63a1487
1110979
8eac9cb
830c991
7a0a44c
6a55c6f
1119041
f56a05d
cb52fd7
94f1bf7
b63147a
3613f22
2b8d534
895bbc4
538d003
1eea5d2
d91716b
244b83b
c85ebdc
204b2b9
707b1bd
5e0f599
f888ecf
3c8da66
40479c1
46e5d1b
db8ba58
8fe8c07
aa5fc4f
9fce34b
1440215
89f8116
5dd25a2
676a636
23c28af
ad69e11
f6cfcf3
2d3abe8
cfa01b6
e5b8e58
fafcb9b
e80194f
e0f1a40
8bcbb64
1e0dde3
7cae57b
1002fc6
a33aa8a
7900876
46b7b3e
e54fd45
a51bb40
fdc74c7
9a35f1a
ef20d55
65ca5a9
ff234b4
72d82d6
3e5931d
67e0f7b
a6e1a8f
9321f4e
e427e82
2e81a5c
ae9657d
a86f597
71b025e
2637165
fd897b3
7400aa4
bbb0b19
e82a49c
4e42254
971de66
2c05a81
f0af395
dd3e972
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,10 @@ | |
color: $black; | ||
} | ||
|
||
.h-200px { | ||
height: 200px; | ||
} | ||
|
||
.mw-300px { | ||
max-width: 300px; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
import React from 'react'; | ||
import { render } from '@testing-library/react'; | ||
|
||
import LoadingButton from '.'; | ||
|
||
const buttonTitle = 'Button Title'; | ||
|
||
const RootWrapper = (onClick) => ( | ||
<LoadingButton onClick={onClick}> | ||
{buttonTitle} | ||
</LoadingButton> | ||
); | ||
|
||
describe('<LoadingButton />', () => { | ||
it('renders the title and doesnt handle the spinner initially', () => { | ||
const { getByText, queryByTestId } = render(RootWrapper(() => { })); | ||
const titleElement = getByText(buttonTitle); | ||
expect(titleElement).toBeInTheDocument(); | ||
expect(queryByTestId('button-loading-spinner')).not.toBeInTheDocument(); | ||
}); | ||
|
||
it('doesnt render the spinner initially without onClick function', () => { | ||
const { getByRole, getByText, queryByTestId } = render(RootWrapper()); | ||
const titleElement = getByText(buttonTitle); | ||
expect(titleElement).toBeInTheDocument(); | ||
expect(queryByTestId('button-loading-spinner')).not.toBeInTheDocument(); | ||
const buttonElement = getByRole('button'); | ||
buttonElement.click(); | ||
expect(queryByTestId('button-loading-spinner')).not.toBeInTheDocument(); | ||
}); | ||
|
||
it('renders the spinner correctly', () => { | ||
const longFunction = () => new Promise((resolve) => { | ||
setTimeout(resolve, 1000); | ||
}); | ||
const { getByRole, getByText, getByTestId } = render(RootWrapper(longFunction)); | ||
const buttonElement = getByRole('button'); | ||
buttonElement.click(); | ||
const spinnerElement = getByTestId('button-loading-spinner'); | ||
expect(spinnerElement).toBeInTheDocument(); | ||
const titleElement = getByText(buttonTitle); | ||
expect(titleElement).toBeInTheDocument(); | ||
expect(buttonElement).toBeDisabled(); | ||
setTimeout(() => { | ||
expect(buttonElement).toBeEnabled(); | ||
expect(spinnerElement).not.toBeInTheDocument(); | ||
}, 2000); | ||
}); | ||
|
||
it('renders the spinner correctly even with error', () => { | ||
const longFunction = () => new Promise((_resolve, reject) => { | ||
setTimeout(reject, 1000); | ||
}); | ||
const { getByRole, getByText, getByTestId } = render(RootWrapper(longFunction)); | ||
const buttonElement = getByRole('button'); | ||
buttonElement.click(); | ||
const spinnerElement = getByTestId('button-loading-spinner'); | ||
expect(spinnerElement).toBeInTheDocument(); | ||
const titleElement = getByText(buttonTitle); | ||
expect(titleElement).toBeInTheDocument(); | ||
expect(buttonElement).toBeDisabled(); | ||
setTimeout(() => { | ||
expect(buttonElement).toBeEnabled(); | ||
expect(spinnerElement).not.toBeInTheDocument(); | ||
}, 2000); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
// @ts-check | ||
import React, { useState } from 'react'; | ||
|
||
import { | ||
Button, | ||
Spinner, | ||
Stack, | ||
} from '@edx/paragon'; | ||
|
||
/** | ||
* A button that shows a loading spinner when clicked. | ||
* @param {object} props | ||
* @param {React.ReactNode=} props.children | ||
* @param {boolean=} props.disabled | ||
* @param {function=} props.onClick | ||
* @returns {JSX.Element} | ||
*/ | ||
const LoadingButton = ({ | ||
onClick, | ||
children, | ||
disabled, | ||
...props | ||
}) => { | ||
const [isLoading, setIsLoading] = useState(false); | ||
|
||
const loadingOnClick = async (e) => { | ||
if (!onClick) { | ||
return; | ||
} | ||
|
||
setIsLoading(true); | ||
try { | ||
await onClick(e); | ||
} finally { | ||
setIsLoading(false); | ||
} | ||
}; | ||
|
||
return ( | ||
<Button | ||
{...props} | ||
disabled={!!isLoading || disabled} | ||
onClick={loadingOnClick} | ||
> | ||
<Stack gap={2} direction="horizontal"> | ||
{children} | ||
{isLoading && <Spinner size="sm" animation="border" data-testid="button-loading-spinner" />} | ||
</Stack> | ||
</Button> | ||
); | ||
}; | ||
|
||
LoadingButton.propTypes = { | ||
...Button.propTypes, | ||
}; | ||
|
||
LoadingButton.defaultProps = { | ||
...Button.defaultProps, | ||
}; | ||
|
||
export default LoadingButton; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,32 +1,51 @@ | ||
// @ts-check | ||
import React, { useMemo, useState } from 'react'; | ||
import { StudioFooter } from '@edx/frontend-component-footer'; | ||
import { useIntl } from '@edx/frontend-platform/i18n'; | ||
import { Outlet, ScrollRestoration } from 'react-router-dom'; | ||
import { Toast } from '@edx/paragon'; | ||
|
||
import AlertMessage from '../generic/alert-message'; | ||
import Header from '../header'; | ||
import { TaxonomyContext } from './common/context'; | ||
import messages from './messages'; | ||
|
||
const TaxonomyLayout = () => { | ||
const intl = useIntl(); | ||
// Use `setToastMessage` to show the toast. | ||
const [toastMessage, setToastMessage] = useState(null); | ||
// Use `setToastMessage` to show the alert. | ||
const [alertProps, setAlertProps] = useState(null); | ||
|
||
const context = useMemo(() => ({ | ||
toastMessage, setToastMessage, | ||
toastMessage, setToastMessage, alertProps, setAlertProps, | ||
}), []); | ||
|
||
return ( | ||
<TaxonomyContext.Provider value={context}> | ||
<div className="bg-light-400"> | ||
<Header isHiddenMainMenu /> | ||
{ alertProps && ( | ||
<AlertMessage | ||
data-testid="taxonomy-alert" | ||
className="mb-0" | ||
dismissible | ||
closeLabel={intl.formatMessage(messages.taxonomyDismissLabel)} | ||
onClose={() => setAlertProps(null)} | ||
// @ts-ignore ToDo: fix object spread type error | ||
{...alertProps} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this todo intentional? Is it intended to be fixed in a future PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. I was having trouble fixing this type check before. It was happening because I was missing the state type. |
||
/> | ||
)} | ||
<Outlet /> | ||
<StudioFooter /> | ||
<Toast | ||
show={toastMessage !== null} | ||
onClose={() => setToastMessage(null)} | ||
data-testid="taxonomy-toast" | ||
> | ||
{toastMessage} | ||
</Toast> | ||
{toastMessage && ( | ||
<Toast | ||
onClose={() => setToastMessage(null)} | ||
data-testid="taxonomy-toast" | ||
> | ||
{toastMessage} | ||
</Toast> | ||
)} | ||
</div> | ||
<ScrollRestoration /> | ||
</TaxonomyContext.Provider> | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,32 +1,50 @@ | ||||||
import React from 'react'; | ||||||
import React, { useContext } from 'react'; | ||||||
import { IntlProvider } from '@edx/frontend-platform/i18n'; | ||||||
import { initializeMockApp } from '@edx/frontend-platform'; | ||||||
import { AppProvider } from '@edx/frontend-platform/react'; | ||||||
import { render, act } from '@testing-library/react'; | ||||||
import { render } from '@testing-library/react'; | ||||||
|
||||||
import initializeStore from '../store'; | ||||||
import { TaxonomyContext } from './common/context'; | ||||||
import TaxonomyLayout from './TaxonomyLayout'; | ||||||
|
||||||
let store; | ||||||
const toastMessage = 'Hello, this is a toast!'; | ||||||
const alertErrorTitle = 'Error title'; | ||||||
const alertErrorDescription = 'Error description'; | ||||||
|
||||||
const MockChildComponent = () => { | ||||||
const { setToastMessage, setAlertProps } = useContext(TaxonomyContext); | ||||||
|
||||||
return ( | ||||||
<div data-testid="mock-content"> | ||||||
<button | ||||||
type="button" | ||||||
onClick={() => setToastMessage(toastMessage)} | ||||||
data-testid="taxonomy-show-toast" | ||||||
> | ||||||
Show Toast | ||||||
</button> | ||||||
<button | ||||||
type="button" | ||||||
onClick={() => setAlertProps({ title: alertErrorTitle, description: alertErrorDescription })} | ||||||
data-testid="taxonomy-show-alert" | ||||||
> | ||||||
Show Alert | ||||||
</button> | ||||||
</div> | ||||||
); | ||||||
}; | ||||||
|
||||||
jest.mock('../header', () => jest.fn(() => <div data-testid="mock-header" />)); | ||||||
jest.mock('@edx/frontend-component-footer', () => ({ | ||||||
StudioFooter: jest.fn(() => <div data-testid="mock-footer" />), | ||||||
})); | ||||||
jest.mock('react-router-dom', () => ({ | ||||||
...jest.requireActual('react-router-dom'), | ||||||
Outlet: jest.fn(() => <div data-testid="mock-content" />), | ||||||
Outlet: () => <MockChildComponent />, | ||||||
ScrollRestoration: jest.fn(() => <div />), | ||||||
})); | ||||||
jest.mock('react', () => ({ | ||||||
...jest.requireActual('react'), | ||||||
useState: jest.fn((initial) => { | ||||||
if (initial === null) { | ||||||
return [toastMessage, jest.fn()]; | ||||||
} | ||||||
return [initial, jest.fn()]; | ||||||
}), | ||||||
})); | ||||||
|
||||||
const RootWrapper = () => ( | ||||||
<AppProvider store={store}> | ||||||
|
@@ -49,18 +67,37 @@ describe('<TaxonomyLayout />', async () => { | |||||
store = initializeStore(); | ||||||
}); | ||||||
|
||||||
it('should render page correctly', async () => { | ||||||
it('should render page correctly', () => { | ||||||
const { getByTestId } = render(<RootWrapper />); | ||||||
expect(getByTestId('mock-header')).toBeInTheDocument(); | ||||||
expect(getByTestId('mock-content')).toBeInTheDocument(); | ||||||
expect(getByTestId('mock-footer')).toBeInTheDocument(); | ||||||
}); | ||||||
|
||||||
it('should show toast', async () => { | ||||||
it('should show toast', () => { | ||||||
const { getByTestId, getByText } = render(<RootWrapper />); | ||||||
act(() => { | ||||||
expect(getByTestId('taxonomy-toast')).toBeInTheDocument(); | ||||||
expect(getByText(toastMessage)).toBeInTheDocument(); | ||||||
}); | ||||||
const button = getByTestId('taxonomy-show-toast'); | ||||||
button.click(); | ||||||
expect(getByTestId('taxonomy-toast')).toBeInTheDocument(); | ||||||
expect(getByText(toastMessage)).toBeInTheDocument(); | ||||||
}); | ||||||
|
||||||
it('should show alert', () => { | ||||||
const { | ||||||
getByTestId, | ||||||
getByText, | ||||||
getByRole, | ||||||
queryByTestId, | ||||||
} = render(<RootWrapper />); | ||||||
|
||||||
const button = getByTestId('taxonomy-show-alert'); | ||||||
button.click(); | ||||||
expect(getByTestId('taxonomy-alert')).toBeInTheDocument(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of button.click etc, you should ideally use fireEvent.click(button) or userEvent.click(button)
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done: 71b025e |
||||||
expect(getByText(alertErrorTitle)).toBeInTheDocument(); | ||||||
expect(getByText(alertErrorDescription)).toBeInTheDocument(); | ||||||
|
||||||
const closeAlertButton = getByRole('button', { name: 'Dismiss' }); | ||||||
closeAlertButton.click(); | ||||||
expect(queryByTestId('taxonomy-alert')).not.toBeInTheDocument(); | ||||||
}); | ||||||
}); |
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 is already a component like this in Paragon called
StatefulButton
.I think the use case here could be covered by that component.
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 is a bit different. The StatefulButton shows the state, but the state resides outside the component. The LoadingButton also handles the state inside according to the onClick call, abstracting this from the developer.
I changed the LoadingButton to use the StatefulButton inside here bbb0b19. If you don't agree to add another component let me know, and I will change it to a standard button to avoid more boiler plate in this already big component.