-
Notifications
You must be signed in to change notification settings - Fork 108
feat: library unit page skeleton [FC-0083] #1779
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: library unit page skeleton [FC-0083] #1779
Conversation
Thanks for the pull request, @navinkarkera! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
f285efc
to
f33c56b
Compare
8fe30f4
to
f561cfc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1779 +/- ##
==========================================
+ Coverage 93.52% 93.56% +0.03%
==========================================
Files 1130 1136 +6
Lines 23029 23205 +176
Branches 4864 5000 +136
==========================================
+ Hits 21539 21711 +172
+ Misses 1422 1418 -4
- Partials 68 76 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7dec047
to
4d8978f
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.
👍 This works beautifully, thank you @navinkarkera ! I noted a couple things, but nothing that needs to be addressed before merge.
- I tested this using a Unit with and without components (responsive iframe sizing works great), non-unit Component sidebar previews and preview modals, and course Unit preview iframes (all still work great).
- I read through the code
- I checked for accessibility issues by using my keyboard to navigate -- appreciate the visual cues for hover/select unit components!
- Includes documentation -- code comments
- User-facing strings are extracted for translation
}: LibraryBlockProps) => { | ||
const iframeRef = useRef<HTMLIFrameElement>(null); | ||
const { iframeRef, setIframeRef } = useIframe(); |
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.
Great cleanup here -- thank you!
defaultTab: DefaultTabs; | ||
setDefaultTab: (tabs: DefaultTabs) => void; | ||
disabledTabs: Array<SidebarInfoTab>; | ||
setDisabledTabs: (tabs: ComponentInfoTab[]) => void; |
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.
nit: I would have called these "hidden tabs" instead of "disabled".
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.
Updated: c14509d
@@ -154,6 +156,18 @@ const ComponentInfo = () => { | |||
}); | |||
}, [publishComponent, showToast, intl]); | |||
|
|||
const renderTab = React.useCallback((infoTab: ComponentInfoTab, component: React.ReactNode, title: string) => { | |||
if (disabledTabs.includes(infoTab)) { | |||
// For some reason, returning anything other than empty list breaks the tab style |
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.
TODO for later: refactor sidebar Tabs
to handle rendering and disabledTabs in one place.
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.
Added todo here: c14509d
src/library-authoring/routes.ts
Outdated
@@ -122,18 +127,23 @@ export const useLibraryRoutes = (): LibraryRoutesData => { | |||
// We're inside the Units tab, so stay there, | |||
// optionally selecting a unit. | |||
route = ROUTES.UNITS; |
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 cannot Open a Unit page from within the Units tab -- works fine from All Content tab.
(Ok to leave this bug for a follow-up 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.
Fixed it here: a40b18c
a2f3b19
to
a40b18c
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! Only one nit
} from './hooks'; | ||
import { | ||
XBlockContainerIframeProps, | ||
AccessManagedXBlockDataTypes, | ||
} from './types'; | ||
import { formatAccessManagedXBlockData, getIframeUrl, getLegacyEditModalUrl } from './utils'; | ||
import messages from './messages'; | ||
import { useIFrameBehavior } from '../../generic/hooks/useIFrameBehavior'; |
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.
@navinkarkera Could you update to useIframeBehavior
? To keep the same format as useIframeContent
and useIframeMessages
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. 7daad72
c14509d
to
ea853aa
Compare
@navinkarkera @ChrisChV I'd like to merge this ASAP. Is it OK to merge even before openedx/edx-platform#36477 is ready, or is it blocked on that PR? It's fine if there's some minor issues with rendering unit contents until the other PR is merged. |
@bradenmacdonald it depends on the platform PR, so we need to merge it first |
Description: Unit page with following features:
Current state: Note: drag and drop is not persistent, just the frontend
vokoscreenNG-2025-04-09_18-51-06.webm
Test instructions: