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.

Sign up for GitHub

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

🧱 Simplify Blocks (remove grid-system) #548

Merged
merged 1 commit into from
Mar 4, 2025
Merged

🧱 Simplify Blocks (remove grid-system) #548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions .changeset/smart-drinks-explain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'myst-to-react': patch
'@myst-theme/jupyter': patch
---

Simplify blocks
46 changes: 10 additions & 36 deletions packages/jupyter/src/block.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,45 +7,30 @@ import {
NotebookRunCell,
NotebookRunCellSpinnerOnly,
} from './controls/index.js';
import { useGridSystemProvider, usePageKind } from '@myst-theme/providers';
import type { NodeRenderers, NodeRenderer } from '@myst-theme/providers';
import { usePageKind } from '@myst-theme/providers';
import type { NodeRenderers } from '@myst-theme/providers';

export function NotebookBlock({
id,
node,
className,
}: {
id: string;
node: GenericParent;
className?: string;
}) {
export function NotebookBlock({ node, className }: { node: GenericParent; className?: string }) {
const pageKind = usePageKind();
const grid = useGridSystemProvider();
const subGrid = node.visibility === 'hide' ? '' : `${grid} subgrid-gap col-screen`;
const dataClassName = typeof node.data?.class === 'string' ? node.data?.class : undefined;
// Hide the subgrid if either the dataClass or the className exists and includes `col-`
const noSubGrid =
(dataClassName && dataClassName.includes('col-')) || (className && className.includes('col-'));
const block = (
<div
key={`block-${id}`}
id={id}
className={classNames('relative group/block', className, dataClassName, {
[subGrid]: !noSubGrid,
id={node.key}
className={classNames('relative group/block', className, node.class, {
[node.data?.class]: typeof node.data?.class === 'string',
hidden: node.visibility === 'remove',
})}
>
{pageKind === SourceFileKind.Notebook && node.kind === 'notebook-code' && (
<>
<div className="flex sticky top-[80px] z-10 opacity-70 group-hover/block:opacity-100 group-hover/block:hidden">
<div className="absolute top-0 -right-[28px] flex md:flex-col">
<NotebookRunCellSpinnerOnly id={id} />
<NotebookRunCellSpinnerOnly id={node.key} />
</div>
</div>
<div className="hidden sticky top-[80px] z-10 opacity-70 group-hover/block:opacity-100 group-hover/block:flex">
<div className="absolute top-0 -right-[28px] flex md:flex-col">
<NotebookRunCell id={id} />
<NotebookClearCell id={id} />
<NotebookRunCell id={node.key} />
<NotebookClearCell id={node.key} />
</div>
</div>
</>
Expand All @@ -59,17 +44,6 @@ export function NotebookBlock({
return block;
}

export const NotebookBlockRenderer: NodeRenderer = ({ node, className }) => {
return (
<NotebookBlock
key={node.key}
id={node.key}
node={node}
className={classNames(node.class, className)}
/>
);
};

/**
* The logic for the selector is complex:
*
Expand All @@ -83,6 +57,6 @@ export const NotebookBlockRenderer: NodeRenderer = ({ node, className }) => {
*/
export const NOTEBOOK_BLOCK_RENDERERS: NodeRenderers = {
block: {
'block[kind=notebook-code],block[kind=notebook-content]': NotebookBlockRenderer,
'block[kind=notebook-code],block[kind=notebook-content]': NotebookBlock,
},
};
50 changes: 16 additions & 34 deletions packages/myst-to-react/src/block.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,50 +2,32 @@ import { Details } from './dropdown.js';
import { MyST } from './MyST.js';
import type { GenericParent } from 'myst-common';
import classNames from 'classnames';
import { useGridSystemProvider } from '@myst-theme/providers';
import type { NodeRenderer } from '@myst-theme/providers';

export function Block({
id,
node,
className,
}: {
id: string;
node: GenericParent;
className?: string;
}) {
const grid = useGridSystemProvider();
const subGrid = node.visibility === 'hide' ? '' : `${grid} subgrid-gap col-screen`;
const dataClassName = typeof node.data?.class === 'string' ? node.data?.class : undefined;
// Hide the subgrid if either the dataClass or the className exists and includes `col-`
const noSubGrid =
(dataClassName && dataClassName.includes('col-')) || (className && className.includes('col-'));
const block = (
<div
key={`block-${id}`}
id={id}
className={classNames('relative group/block', className, dataClassName, {
[subGrid]: !noSubGrid,
hidden: node.visibility === 'remove',
})}
>
export function Block({ node, className }: { node: GenericParent; className?: string }) {
const cn = classNames(className, node.class, {
[node.data?.class]: typeof node.data?.class === 'string',
});
if (node.visibility === 'remove') return null;
// Only wrap this in a block if the block has a class name or an identifier
// Otherwise pass through the contents as is.
// This allows children (e.g. margin) access to the grid-system
const block =
cn || node.identifier ? (
<div id={node.identifier} className={cn}>
Comment on lines +15 to +17
Copy link
Collaborator

@agoose77 agoose77 Mar 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@rowanc1 I don't think this change is friendly to future outputs work or #540:

Elements like aside need the CSS grid to position themselves correctly. This is only possible if they are top-level elements within a grid, or their parents use subgrid. Previously, we ensured that the outermost block created a subgrid.

This PR drops the subgrid feature by only wrapping the block children in a node if they have a class or an identifier. That fixes the https://cpraveen.github.io/numa/stiff1 example, but has the following shortcoming: Both code-outputs that generate e.g. an aside and blocks that use #540 will set an identifier and/or class, but want to preserve the grid system.

I think instead we probably need to retain the grid e.g. #547.

<MyST ast={node.children} />
</div>
) : (
<MyST ast={node.children} />
</div>
);
);
if (node.visibility === 'hide') {
return <Details title="Block">{block}</Details>;
}
return block;
}

export const BlockRenderer: NodeRenderer = ({ node, className }) => {
return (
<Block key={node.key} id={node.key} node={node} className={classNames(node.class, className)} />
);
};

const BLOCK_RENDERERS: Record<string, NodeRenderer> = {
block: BlockRenderer,
block: Block,
};

export default BLOCK_RENDERERS;
Loading