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.

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

Minimal Library Asset Support #1383

Merged
merged 2 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 17 additions & 1 deletion src/editors/sharedComponents/TinyMceWidget/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import pluginConfig from './pluginConfig';
import * as module from './hooks';
import * as tinyMCE from '../../data/constants/tinyMCE';
import { getRelativeUrl, getStaticUrl, parseAssetName } from './utils';
import { isLibraryKey } from '../../../generic/key-utils';

export const state = StrictDict({
// eslint-disable-next-line react-hooks/rules-of-hooks
Expand Down Expand Up @@ -83,6 +84,20 @@ export const replaceStaticWithAsset = ({
editorType,
lmsEndpointUrl,
}) => {
if (isLibraryKey(learningContextId)) {
// This function doesn't currently know how to deal with Library assets.
// Libraries don't mangle the path into an asset key–it might be sufficient
// to remove the initial "/" in a "/static/images/foo.png" link, and then
// set the base URL to the correct ComponentVersion base. If we let this
// function try to deal with Library assets, it would convert them in such a
// way that it wouldn't convert back later on, and we'd end up storing the
// incorrect OLX and breaking the display from that point forward.
//
// So until we handle it better, just disable static asset URL substitutions
// when dealing with Library content.
return false;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bradenmacdonald, @kdmccormick: I changed this to be a simpler early return–since I realized that this function returns false if it finds no matching static URLs.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM!


let content = initialContent;
let hasChanges = false;
const srcs = content.split(/(src="|src="|href="|href=&quot)/g).filter(
Expand All @@ -98,7 +113,8 @@ export const replaceStaticWithAsset = ({
const assetName = parseAssetName(src);
const displayName = isStatic ? staticName : assetName;
const isCorrectAssetFormat = assetSrc.startsWith('/asset') && assetSrc.match(/\/asset-v1:\S+[+]\S+[@]\S+[+]\S+[@]/g)?.length >= 1;
// assets in expandable text areas so not support relative urls so all assets must have the lms

// assets in expandable text areas do not support relative urls so all assets must have the lms
// endpoint prepended to the relative url
if (editorType === 'expandable') {
if (isCorrectAssetFormat) {
Expand Down
4 changes: 2 additions & 2 deletions src/library-authoring/LibraryBlock/LibraryBlock.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ interface LibraryBlockProps {
const LibraryBlock = ({ onBlockNotification, usageKey }: LibraryBlockProps) => {
const iframeRef = useRef<HTMLIFrameElement>(null);
const [iFrameHeight, setIFrameHeight] = useState(600);
const lmsBaseUrl = getConfig().LMS_BASE_URL;
const studioBaseUrl = getConfig().STUDIO_BASE_URL;

const intl = useIntl();

Expand Down Expand Up @@ -74,7 +74,7 @@ const LibraryBlock = ({ onBlockNotification, usageKey }: LibraryBlockProps) => {
<iframe
ref={iframeRef}
title={intl.formatMessage(messages.iframeTitle)}
src={`${lmsBaseUrl}/xblocks/v2/${usageKey}/embed/student_view/`}
src={`${studioBaseUrl}/xblocks/v2/${usageKey}/embed/student_view/`}
data-testid="block-preview"
style={{
width: '100%',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe('<ComponentPreview />', () => {
const usageKey = mockLibraryBlockMetadata.usageKeyPublished;
render(<ComponentPreview usageKey={usageKey} />);
const iframe = (await screen.findByTitle('Preview')) as HTMLIFrameElement;
expect(iframe.src).toEqual(`http://localhost:18000/xblocks/v2/${usageKey}/embed/student_view/`);
expect(iframe.src).toEqual(`http://localhost:18010/xblocks/v2/${usageKey}/embed/student_view/`);
});

it('shows an expanded preview of the component', async () => {
Expand All @@ -30,6 +30,6 @@ describe('<ComponentPreview />', () => {
const dialogIframe = dialog.querySelector('iframe')!;
expect(dialogIframe).not.toBeNull();
expect(dialogIframe).toHaveAttribute('title', 'Preview');
expect(dialogIframe.src).toEqual(`http://localhost:18000/xblocks/v2/${usageKey}/embed/student_view/`);
expect(dialogIframe.src).toEqual(`http://localhost:18010/xblocks/v2/${usageKey}/embed/student_view/`);
});
});