-
Notifications
You must be signed in to change notification settings - Fork 0
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
refactor: [AXIMST-658] Course unit - Copy/paste refactoring #204
Conversation
3185b66
to
367946f
Compare
910b0bd
to
19e8989
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## ts-develop #204 +/- ##
=============================================
Coverage ? 89.51%
=============================================
Files ? 648
Lines ? 10839
Branches ? 2252
=============================================
Hits ? 9702
Misses ? 1098
Partials ? 39 ☔ View full report in Codecov by Sentry. |
19e8989
to
46ab5bb
Compare
46ab5bb
to
c7d6a1e
Compare
dd8a0f9
to
509070d
Compare
const popoverContent = queryByTestId('popover-content'); | ||
const apiBaseUrl = getConfig().STUDIO_BASE_URL; | ||
expect(popoverContent.tagName).toBe('A'); | ||
expect(popoverContent).toHaveAttribute('href', apiBaseUrl + unit.studioUrl); |
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.
expect(popoverContent).toHaveAttribute('href', apiBaseUrl + unit.studioUrl); | |
expect(popoverContent).toHaveAttribute('href', `${getConfig().STUDIO_BASE_URL}${unit.studioUrl}`); |
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.
Corrected
* @returns {Object} - An object containing state variables and functions related to clipboard functionality. | ||
* @property {boolean} showPasteUnit - Flag indicating whether the "Paste Unit" button should be visible. | ||
* @property {boolean} showPasteXBlock - Flag indicating whether the "Paste XBlock" button should be visible. | ||
* @property {Object} sharedClipboardData - The shared clipboard data object. |
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.
👍
const [clipboardBroadcastChannel] = useState(() => new BroadcastChannel(STUDIO_CLIPBOARD_CHANNEL)); | ||
const [showPasteUnit, setShowPasteUnit] = useState(false); | ||
const [showPasteXBlock, setShowPasteXBlock] = useState(false); | ||
const clipboardData = useSelector(getClipboardData); |
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.
why do we use this selector outside of the hook and pass as param?
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.
can it be application wide state?
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.
Conducted refactoring and moved the redux logic associated with the copy/paste functionality into the generic slice
import { clipboardPropsTypes, OVERLAY_TRIGGERS } from './constants'; | ||
|
||
const PasteComponent = ({ handleCreateNewCourseXBlock, clipboardData }) => { | ||
const PasteButton = ({ |
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.
Maybe this we can call as ClipboardPasteComponent
and inside we will have
ClipboardPasteButton`?
Component - because it consist of other parts like button in this case and etc.
Button - single element that will a part of 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.
Renamed
fc730d3
to
549f3ac
Compare
549f3ac
to
6ebd1ad
Compare
import PropTypes from 'prop-types'; | ||
import { Warning as WarningIcon } from '@openedx/paragon/icons'; | ||
import { useIntl } from '@edx/frontend-platform/i18n'; | ||
|
||
import { RequestStatus } from '../../data/constants'; | ||
import { getSavingStatus } from '../data/selectors'; |
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.
Maybe here we will use the same alias?
import { getSavingStatus } from '../data/selectors'; | |
import { getSavingStatus as getGenericSavingStatus } from '../data/selectors'; |
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.
Renamed, thanks
* refactor: copy paste functional refactoring * refactor: refactoring paste-button * refactor: tests refactoring * refactor: updated translations * refactor: refactoring after review * refactor: renamed status selector name
* refactor: copy paste functional refactoring * refactor: refactoring paste-button * refactor: tests refactoring * refactor: updated translations * refactor: refactoring after review * refactor: renamed status selector name
* refactor: copy paste functional refactoring * refactor: refactoring paste-button * refactor: tests refactoring * refactor: updated translations * refactor: refactoring after review * refactor: renamed status selector name
* refactor: copy paste functional refactoring * refactor: refactoring paste-button * refactor: tests refactoring * refactor: updated translations * refactor: refactoring after review * refactor: renamed status selector name
YouTrack