Add an "ide" component - #13708
Conversation
|
Staged preview of the updated flutter.dev site (updated for commit 68a0f0c): https://flutter-dev-230821--www-pr13708-ide-component-djgv8rr8.web.app |
|
Staged preview of the updated docs.flutter.dev site (updated for commit 68a0f0c): https://flutter-docs-prod--docs-pr13708-ide-component-1g9w9f0b.web.app |
There was a problem hiding this comment.
Code Review
This pull request introduces a new interactive file-tree explorer component (IdeExplorer) for documentation sites, including its SASS styles, Jaspr Dart components, a custom markdown parser, and client-side interactivity scripts. The feedback highlights several critical issues: a potential hydration mismatch in SSR due to a global static counter, invalid HTML nesting of a inside a
| return const Component.empty(); | ||
| } | ||
|
|
||
| final effectiveInstanceId = instanceId ?? '${_nextInstanceId++}'; |
There was a problem hiding this comment.
Using a global static counter (_nextInstanceId) to generate default IDs in a server-rendered (SSR) environment like Jaspr can lead to hydration mismatches. The server's counter will persist across requests and increment, while the client's counter will reset to 0 on page load. This causes the client-side IDs to mismatch the server-rendered IDs, breaking interactivity.
To resolve this, we can generate a stable, deterministic ID based on the roots of the explorer, which will be identical on both server and client.
| final effectiveInstanceId = instanceId ?? '${_nextInstanceId++}'; | |
| final effectiveInstanceId = instanceId ?? | |
| roots.map((r) => r.id).join('_').hashCode.abs().toRadixString(36); |
| final isFolderSelf = | ||
| sidebarTarget?.parentElement?.tagName.toLowerCase() == 'summary'; | ||
| var current = isFolderSelf ? ownDetails?.parentElement : ownDetails; |
There was a problem hiding this comment.
Clicking a folder row currently selects the folder but does not expand/collapse it. This is because event.preventDefault() is called on the click event, and selectIdeNode explicitly skips opening the folder itself when isFolderSelf is true.
To fix this and ensure that selecting a folder also expands it (so users can see its contents), we should always start the expansion loop from ownDetails instead of skipping it.
| final isFolderSelf = | |
| sidebarTarget?.parentElement?.tagName.toLowerCase() == 'summary'; | |
| var current = isFolderSelf ? ownDetails?.parentElement : ownDetails; | |
| var current = ownDetails; |
| classes: ['ide-node-row', if (isSelected) 'active'].toClasses, | ||
| type: ButtonType.button, | ||
| attributes: { | ||
| 'data-ide-select': domId, | ||
| if (isSelected) 'aria-current': 'true', | ||
| }, | ||
| [ | ||
| icon, | ||
| span(classes: 'ide-node-label', [.text(node.label)]), | ||
| _IdeBadgeDot( | ||
| badge: node.badge, | ||
| color: node.badgeColor ?? IdeBadgeColor.neutral, | ||
| ), | ||
| ], | ||
| ); | ||
| } |
There was a problem hiding this comment.
Nesting a <button> inside a <summary> element is invalid HTML and causes accessibility issues (screen readers get confused by nested interactive controls).
Since the <summary> element is already interactive, we should render a <span> instead of a <button> when the node is a folder. For files (which are rendered inside <li>), we can continue using a <button>.
final classesList = ['ide-node-row', if (isSelected) 'active'].toClasses;
final attributesMap = {
'data-ide-select': domId,
if (isSelected) 'aria-current': 'true',
};
final childrenList = [
icon,
span(classes: 'ide-node-label', [.text(node.label)]),
_IdeBadgeDot(
badge: node.badge,
color: node.badgeColor ?? IdeBadgeColor.neutral,
),
];
if (node.isFolder) {
return span(
classes: classesList,
attributes: attributesMap,
childrenList,
);
}
return button(
classes: classesList,
type: ButtonType.button,
attributes: attributesMap,
childrenList,
);| final roots = [ | ||
| for (final (index, rootEl) in projectRootElements.indexed) | ||
| IdeExplorerProjectRoot( | ||
| id: _generateNodeId( | ||
| rootEl.attributes, | ||
| _defaultRootPrefix, | ||
| index, | ||
| ), | ||
| label: rootEl.attributes[_attrLabel] ?? '', | ||
| children: _parseTreeNodes( | ||
| rootEl.children, | ||
| builder, | ||
| customContents, | ||
| ), | ||
| ), | ||
| ]; |
There was a problem hiding this comment.
Refactor the root generation loop to avoid duplicate ID generation and pass the root ID down to _parseTreeNodes to ensure unique IDs across the entire tree.
final roots = <IdeExplorerProjectRoot>[];
for (final (index, rootEl) in projectRootElements.indexed) {
final rootId = _generateNodeId(
rootEl.attributes,
_defaultRootPrefix,
index,
);
roots.add(IdeExplorerProjectRoot(
id: rootId,
label: rootEl.attributes[_attrLabel] ?? '',
children: _parseTreeNodes(
rootEl.children,
builder,
customContents,
rootId,
),
));
}| String _generateNodeId( | ||
| Map<String, String> attributes, | ||
| String prefix, | ||
| int index, | ||
| ) { | ||
| if (attributes[_attrId] != null) { | ||
| return attributes[_attrId]!; | ||
| } | ||
| final label = attributes[_attrLabel]; | ||
| if (label != null && label.isNotEmpty) { | ||
| return slugify(label); | ||
| } | ||
| return '$prefix-$index'; | ||
| } |
There was a problem hiding this comment.
If multiple files or folders in different directories have the same name (e.g., main.dart or README.md), they will generate duplicate IDs because _generateNodeId only uses the label. This causes duplicate DOM IDs and overwrites entries in the customContents map, leading to broken rendering and interactivity.
We can prevent this by making _generateNodeId accept an optional parentId to prefix the generated ID, ensuring uniqueness across different directories.
String _generateNodeId(
Map<String, String> attributes,
String prefix,
int index, [
String? parentId,
]) {
if (attributes[_attrId] != null) {
return attributes[_attrId]!;
}
final label = attributes[_attrLabel];
final localId = (label != null && label.isNotEmpty)
? slugify(label)
: '$prefix-$index';
return parentId != null ? '$parentId-$localId' : localId;
}| List<IdeTreeNode> _parseTreeNodes( | ||
| List<Node>? nodes, | ||
| NodesBuilder builder, | ||
| Map<String, Component> customContents, | ||
| ) { | ||
| if (nodes == null || nodes.isEmpty) return const []; | ||
|
|
||
| final result = <IdeTreeNode>[]; | ||
|
|
||
| for (final (index, child) in nodes.whereType<ElementNode>().indexed) { | ||
| if (child.tag != _tagIdeFolder && child.tag != _tagIdePage) { | ||
| continue; | ||
| } | ||
|
|
||
| final treeNode = _buildTreeNodeFromElement( | ||
| child, | ||
| index, | ||
| builder, | ||
| customContents, | ||
| ); | ||
|
|
||
| result.add(treeNode); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
Update _parseTreeNodes to accept a parentId parameter and pass it down to _buildTreeNodeFromElement to support unique ID generation.
List<IdeTreeNode> _parseTreeNodes(
List<Node>? nodes,
NodesBuilder builder,
Map<String, Component> customContents,
String parentId,
) {
if (nodes == null || nodes.isEmpty) return const [];
final result = <IdeTreeNode>[];
for (final (index, child) in nodes.whereType<ElementNode>().indexed) {
if (child.tag != _tagIdeFolder && child.tag != _tagIdePage) {
continue;
}
final treeNode = _buildTreeNodeFromElement(
child,
index,
builder,
customContents,
parentId,
);
result.add(treeNode);
}
return result;
}| IdeTreeNode _buildTreeNodeFromElement( | ||
| ElementNode element, | ||
| int index, | ||
| NodesBuilder builder, | ||
| Map<String, Component> customContents, | ||
| ) { | ||
| final attributes = element.attributes; | ||
| final label = attributes[_attrLabel] ?? ''; | ||
| final id = _generateNodeId(attributes, _defaultNodePrefix, index); | ||
|
|
||
| // Parse boolean attributes | ||
| final isDefaultPage = _getBoolAttribute( | ||
| attributes, | ||
| _attrIsDefaultPage, | ||
| defaultValue: false, | ||
| ); | ||
| final startsClosed = _getBoolAttribute( | ||
| attributes, | ||
| _attrStartsClosed, | ||
| defaultValue: true, | ||
| ); | ||
|
|
||
| // Parse badge attributes | ||
| final badge = attributes[_attrBadge]; | ||
| final badgeColor = attributes[_attrBadgeColor] != null | ||
| ? IdeBadgeColor.fromString(attributes[_attrBadgeColor]) | ||
| : null; | ||
|
|
||
| final subtitle = attributes[_attrSubtitle]; | ||
|
|
||
| // Recursively parse children | ||
| final nestedTreeNodes = _parseTreeNodes( | ||
| element.children, | ||
| builder, | ||
| customContents, | ||
| ); |
There was a problem hiding this comment.
Update _buildTreeNodeFromElement to accept a parentId parameter, pass it to _generateNodeId, and pass the current id as the parentId to the recursive _parseTreeNodes call.
IdeTreeNode _buildTreeNodeFromElement(
ElementNode element,
int index,
NodesBuilder builder,
Map<String, Component> customContents,
String parentId,
) {
final attributes = element.attributes;
final label = attributes[_attrLabel] ?? '';
final id = _generateNodeId(attributes, _defaultNodePrefix, index, parentId);
// Parse boolean attributes
final isDefaultPage = _getBoolAttribute(
attributes,
_attrIsDefaultPage,
defaultValue: false,
);
final startsClosed = _getBoolAttribute(
attributes,
_attrStartsClosed,
defaultValue: true,
);
// Parse badge attributes
final badge = attributes[_attrBadge];
final badgeColor = attributes[_attrBadgeColor] != null
? IdeBadgeColor.fromString(attributes[_attrBadgeColor])
: null;
final subtitle = attributes[_attrSubtitle];
// Recursively parse children
final nestedTreeNodes = _parseTreeNodes(
element.children,
builder,
customContents,
id,
);
Description of what this PR is changing or adding, and why:
This is adding an interactive IDE component, because its cool. I'm using it for the FlutterBench updates, but want to land this separately to keep the PR sane.
Also adds min/max width breakpoint scss mixin to site-shared. In a future PR, we should remove breakpoints file from sites/www, and also update all scss to use the breakpoints, but doing so here would add many files to this PR.