-
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
feat: refined ux update a taxonomy by downloading and uploading [FC-0036] #732
Conversation
Co-authored-by: Jillian <[email protected]>
* feat: import tags to existing taxonomy * feat: add re-import menu to taxonomy detail * test: add tests * fix: clean debug code Co-authored-by: Jillian <[email protected]> --------- Co-authored-by: Jillian <[email protected]>
* refactor: merges TaxonomyCardMenu and TaxonomyDetailMenu * refactor: change menu item list
Thanks for the pull request, @rpenido! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
2a56eab
to
1f52c40
Compare
1f52c40
to
4c22a0a
Compare
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! I need to test this, but I have a few nits till I reset my devstack and test this.
src/utils.js
Outdated
|
||
export const getFileSizeToClosestByte = (fileSize, numberOfDivides = 0) => { | ||
if (fileSize > 1000) { | ||
const updatedSize = fileSize / 1000; | ||
const incrementNumberOfDivides = numberOfDivides + 1; | ||
return getFileSizeToClosestByte(updatedSize, incrementNumberOfDivides); | ||
} | ||
const fileSizeFixedDecimal = Number.parseFloat(fileSize).toFixed(2); | ||
switch (numberOfDivides) { | ||
case 1: | ||
return `${fileSizeFixedDecimal} KB`; | ||
case 2: | ||
return `${fileSizeFixedDecimal} MB`; | ||
case 3: | ||
return `${fileSizeFixedDecimal} GB`; | ||
default: | ||
return `${fileSizeFixedDecimal} B`; | ||
} | ||
}; |
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.
I think this function is overly complicated for what it's doing.
export const getFileSizeToClosestByte = (fileSize, numberOfDivides = 0) => { | |
if (fileSize > 1000) { | |
const updatedSize = fileSize / 1000; | |
const incrementNumberOfDivides = numberOfDivides + 1; | |
return getFileSizeToClosestByte(updatedSize, incrementNumberOfDivides); | |
} | |
const fileSizeFixedDecimal = Number.parseFloat(fileSize).toFixed(2); | |
switch (numberOfDivides) { | |
case 1: | |
return `${fileSizeFixedDecimal} KB`; | |
case 2: | |
return `${fileSizeFixedDecimal} MB`; | |
case 3: | |
return `${fileSizeFixedDecimal} GB`; | |
default: | |
return `${fileSizeFixedDecimal} B`; | |
} | |
}; | |
export const getFileSizeToClosestByte = (fileSize) => { | |
let divides = 0; | |
let size = fileSize; | |
while (size > 1000) { | |
size /= 1000; | |
divides += 1; | |
} | |
const units = ['B', 'KB', 'MB', 'GB', 'TB']; | |
const fileSizeFixedDecimal = Number.parseFloat(fileSize).toFixed(2); | |
return `${fileSizeFixedDecimal} ${units[divides]}`; | |
}; |
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.
Or using logs:
export const getFileSizeToClosestByte = (fileSize, numberOfDivides = 0) => { | |
if (fileSize > 1000) { | |
const updatedSize = fileSize / 1000; | |
const incrementNumberOfDivides = numberOfDivides + 1; | |
return getFileSizeToClosestByte(updatedSize, incrementNumberOfDivides); | |
} | |
const fileSizeFixedDecimal = Number.parseFloat(fileSize).toFixed(2); | |
switch (numberOfDivides) { | |
case 1: | |
return `${fileSizeFixedDecimal} KB`; | |
case 2: | |
return `${fileSizeFixedDecimal} MB`; | |
case 3: | |
return `${fileSizeFixedDecimal} GB`; | |
default: | |
return `${fileSizeFixedDecimal} B`; | |
} | |
}; | |
export const getFileSizeToClosestByte = (fileSize) => { | |
const units = ['B', 'KB', 'MB', 'GB', 'TB']; | |
const unitPow = Math.floor(Math.log10(fileSize) / 3); | |
const fileSizeFixedDecimal = Number.parseFloat(fileSize / (1000 ** unitPow)).toFixed(2); | |
return `${fileSizeFixedDecimal} ${units[unitPow]}`; | |
}; |
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.
I just moved this code around without putting much thought into it. Refactored here: a86f597
I used the loop solution to add a max value to divide and handle >= 1000 TB without errors.
PS: Very clever log solution! I have never thought of 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.
Yeah I noticed that after typing this up, figured I might as well post it!
src/generic/loading-button/index.jsx
Outdated
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.
/* | ||
className is working on Dropzone: https://github.com/openedx/paragon/pull/2950 | ||
className="h-200px" | ||
*/ |
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.
Thanks for making this fix and including this context!
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.
Unfortunately, upstream is openedx/paragon and we are still using edx/paragon here.
await waitFor(() => { | ||
expect(getByTestId('export-step')).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.
An alternative way to do the same.
await waitFor(() => { | |
expect(getByTestId('export-step')).toBeInTheDocument(); | |
}); | |
expect(await findByTestId('export-step')).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.
Done: 71b025e
src/taxonomy/TaxonomyLayout.jsx
Outdated
// @ts-ignore ToDo: fix object spread type error | ||
{...alertProps} |
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.
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 comment
The 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.
Fixed e82a49c
} = render(<RootWrapper />); | ||
|
||
const button = getByTestId('taxonomy-show-alert'); | ||
button.click(); |
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.
Instead of button.click etc, you should ideally use fireEvent.click(button) or userEvent.click(button)
button.click(); | |
fireEvent.click(button); |
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.
Done: 71b025e
14be854
to
a86f597
Compare
bcbd490
to
62716ab
Compare
62716ab
to
e82a49c
Compare
it('renders the spinner correctly even with error', async () => { | ||
const longFunction = () => new Promise((_resolve, reject) => { | ||
setTimeout(reject, 1000); | ||
}); | ||
const { container, getByRole, getByText } = render(RootWrapper(longFunction)); | ||
const buttonElement = getByRole('button'); | ||
fireEvent.click(buttonElement); | ||
expect(container.getElementsByClassName('icon-spin').length).toBe(1); | ||
expect(getByText(buttonTitle)).toBeInTheDocument(); | ||
// StatefulButton only sets aria-disabled (not disabled) when the state is pending | ||
// expect(buttonElement).toBeDisabled(); | ||
expect(buttonElement).toHaveAttribute('aria-disabled', 'true'); | ||
await waitFor(() => { | ||
// StatefulButton only sets aria-disabled (not disabled) when the state is pending | ||
// expect(buttonElement).toBeEnabled(); | ||
expect(buttonElement).not.toHaveAttribute('aria-disabled', 'true'); | ||
expect(container.getElementsByClassName('icon-spin').length).toBe(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.
This test seems to be throwing an error.
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.
Fixed here: f0af395
it('renders the spinner correctly', async () => { | ||
const longFunction = () => new Promise((resolve) => { | ||
setTimeout(resolve, 1000); | ||
}); | ||
const { container, getByRole, getByText } = render(RootWrapper(longFunction)); | ||
const buttonElement = getByRole('button'); | ||
fireEvent.click(buttonElement); | ||
expect(container.getElementsByClassName('icon-spin').length).toBe(1); | ||
expect(getByText(buttonTitle)).toBeInTheDocument(); | ||
// StatefulButton only sets aria-disabled (not disabled) when the state is pending | ||
// expect(buttonElement).toBeDisabled(); | ||
expect(buttonElement).toHaveAttribute('aria-disabled', 'true'); | ||
await waitFor(() => { | ||
// StatefulButton only sets aria-disabled (not disabled) when the state is pending | ||
// expect(buttonElement).toBeEnabled(); | ||
expect(buttonElement).not.toHaveAttribute('aria-disabled', 'true'); | ||
expect(container.getElementsByClassName('icon-spin').length).toBe(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.
Using setTimeout like this is forcing this function to take 1 second at least, which is about a 100 times slower than it needs to be.
Instead, you can take control of resolving the promise manually to speed things up.
First replace longFunction
with a promise you resolve manually.
let resolve;
let reject;
const longFunction = () => new Promise((resolver, rejecter) => {
resolve = resolver;
reject = rejecter;
});
Now you have the resolve function outside the scope of the promise. So you can add the following:
await act(async () => resolve());
This needs act since it will cause the component to update. Finally you can use the existing final two expect statements, but without the waitFor
.
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.
Thank you @xitij2000! Very clean and elegant.
Solved here: 2c05a81
0675714
to
6082f9a
Compare
6082f9a
to
2c05a81
Compare
2834a54
to
f0af395
Compare
b1163a2
to
dd3e972
Compare
@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This PR improves the import tags functionality for existing taxonomies implemented at #675.
Supporting Information
Testing instructions
Re-import tags to an existing taxonomy
Re-import
menu from an existing taxonomy cardNext
Import
to go to the next step.Continue
. If the file shows no differences, theContinue
button will be disabled.Yes, import file
should import the tags, replacing the old ones.Private-ref: